1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-28 09:42:41 +01:00
Commit graph

2012 commits

Author SHA1 Message Date
epriestley
bf5437212c When a revision is accepted but has open dependencies, show a note in the list UI
Summary:
Ref T10939. I don't think this is hugely important, but it doesn't clutter things up much and it's nice as a hint.

T4055 was the original request specifically asking for this. It wanted a separate bucket, but I think this use case isn't common/strong enough to justify that.

I would like to improve Differential's "X depends on Y" feature in the long term. We don't tend to use/need it much, but it could easily do a better and more automatic job of supporting review of a group of revisions.

Test Plan: {F1426636}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15930
2016-05-16 12:11:52 -07:00
epriestley
c5853b4f48 Put revisions you're a reviewer on which need review and which you've commented on in "Should Review"
Summary: Ref T10939. These poor stragglers got left out in the rain. Didn't catch any issues otherwise.

Test Plan: {F1426604}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15929
2016-05-16 11:39:35 -07:00
epriestley
6e9828c743 Fix a dashboard bucketing bug
Summary: Ref T10939. For various historical reasons, revision status is a numeric string. This comparison fails because it's `(string) !== (int)`. Just use `!=` so this will still work if we turn it into a real string in the future.

Test Plan: Tried a more specific test case locally, got better looking results in "Must Review" and "Should Review".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15928
2016-05-16 11:07:10 -07:00
epriestley
8a98868bfb Remove "days fresh" / "days stale" indictator in Differential revision list
Summary:
Ref T10939. I'm not //totally// opposed to the existence of this element, but I think it's the kind of thing that would never make it upstream today. I think this should just be a T418 custom sort of thing in the long run, not a mainline upstream feature.

Overall, I think this thing is nearly useless and just adds visual clutter. My dashboard is about 100% red. This also sort of teaches users that it's fine to let revisions sit for a couple of days, which isn't what I'd like the UI to teach. Finally, removing it helps the UI feel a little less cluttered after the visually busy changes in D15926.

Test Plan: Grepped for removed config. Viewed revision list.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15927
2016-05-16 10:47:19 -07:00
epriestley
d46378df20 Modernize "Responsible Users" tokenizer and add "exact(user)" token
Summary:
Ref T10939. Fixes T9263. Ref T4144.

First, this resolves users (converting users into all packages and projects they are responsible for) earlier, so bucketing can act on that data correctly. Previously, your own blocking reviews would appear in "Must Review" but your packages/projects' would not. Now, all of them will.

Second, this adds `exact(username)` to mean "just me, not my packages/projects". You can use this along with "Bucket: By Required Action" to create a personal view of "Active Revisions" if you'd like, and ignore all your project/package reviews.

Test Plan: Queried by "me" and "exact(me)", got reasonable looking results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4144, T9263, T10939

Differential Revision: https://secure.phabricator.com/D15925
2016-05-16 10:46:26 -07:00
epriestley
42d49be47b Change Differential revision buckets to focus on "next required action"
Summary:
Ref T10939. Ref T4144. This splits the existing buckets ("Blocking Others", "Action Required", "Waiting on Others") into 6-7 buckets with a stronger focus on what the next action you need to take is.

See T10939#175423 for some discussion.

Overall, I think some of the root problems here are caused by reviewer laziness and shotgun review workflows (where a ton of people get automatically added to everything, probably unnecessarily), but these buckets haven't been updated since the introduction of blocking reviewers or project/package reviewers and I think splitting the 3 buckets into 6 buckets isn't unreasonable, even though it's kind of a lot of buckets and the root problem here is approximately "I want to ignore a bunch of stuff on my dashboard".

I didn't remove the old bucketing code yet since it's still in use on the default homepage.

This also isn't quite right until I fix the tokenizer to work properly, since it won't bucket project/package reviewers accurately.

Test Plan: {F1395972}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4144, T10939

Differential Revision: https://secure.phabricator.com/D15924
2016-05-16 10:45:20 -07:00
epriestley
eade206625 Introduce search result buckets
Summary:
Ref T10939. Currently, Differential hard-codes some behaviors for the "active" filter. This introduces "buckets" to make this grouping behavior more general/flexible.

The buckets don't actually do any grouping yet, this just gets rid of the `$query === 'active'` stuff so far.

These buckets change the page size to a large value, becuase pagination won't currently work with bucketing.

The problem is that we normally paginate by selecting one more result than we need: so if we're building a page of size 10, we'll select 11 results. This is fast, and if we get 11 back, we know there's a next page with at least one result on it.

With buckets, we can't do this, since our 11 results might come back in these buckets:

  - A, B, C, A, C, C, A, A, B, B, (B)

So we know there are more results, and we know that bucket B has more results, but we have no clue if bucket A and bucket C have more results or not (or if there's anything in bucket D, etc).

We might need to select a thousand more results to get the first (D) or the next (A).

So we could render something like "Some buckets have more results, click here to go to the next page", but users would normally expect to be able to see "This specific bucket, A, has more results.", and we can't do that without a lot more work.

It doesn't really matter for revisions, because almost no one has 1K of them, but this may need to be resolved eventually.

(I have some OK-ish ideas for resolving it but nothing I'm particularly happy with.)

Test Plan: {F1376542}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15923
2016-05-16 10:44:42 -07:00
epriestley
3a727c31e2 Modernize DifferentialRevisionSearchEngine
Summary:
Ref T10939. Ref T4144. This moves the revision SearchEngine to modern code so I can add some kind of bucketing layer on top of it.

This seems to have worked pretty cleanly. One thing is that I removed the ability to search for "pending drafts":

  - This was added in D1927 from a bootcamp task, was an indirect solution to a questionable problem, and almost certainly would not meet the bar today.
  - Later, in D3324, we added the icons to the list. I think this is a better solution in general. In particular, it specifically addressed the query being kind of junky.
  - At the time, Differential had a prebuilt "Drafts" filter. This was removed in D6347 with the move to ApplicationSearch, which simplified the large number of prebuilt filters. Although we got a lot of feedback about that, none requested that the drafts filter be restored.

Test Plan: Searched for responsible users, subscribers, orders, projects, repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4144, T10939

Differential Revision: https://secure.phabricator.com/D15921
2016-05-16 10:44:11 -07:00
epriestley
c9365e48d8 Don't trigger "Auto Review" if the author is already an owner; document "Auto Review"
Summary:
Ref T10939. If you already own a package, don't trigger the subscribe/review rules.

Document how these rules work.

Test Plan:
  - Read documentation.
  - Removed reviewers, updated a revision, got autoreviewed.
  - Joined package.
  - Removed reveiwers, updated a revision, no more autoreview.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15918
2016-05-13 17:24:33 -07:00
epriestley
92b9fa47d0 Allow Herald to add package reviewers
Summary: Ref T10939. Packages are valid reviewers, so let Herald "Add Reviewers" and "Add Blocking Reviewers" actions add them.

Test Plan:
  - Wrote a rule to add package reviewers.
  - Hit the rule, saw a package reviewer added, viewed transcript.

{F1311731}

{F1311732}

{F1311733}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15917
2016-05-13 17:23:07 -07:00
epriestley
332d787dc8 Support "Review Changes" and "Block Changes" settings for Owners package "Auto Review"
Summary:
Ref T10939. Fixes T8887. This enables and implements the "review" and "blocking review" options for packages.

This is a bit copy-pastey from `DifferentialReviewersHeraldAction`, which doesn't feel awesome. I think the right fix is Glorious Infrasturcture, though -- I filed T10967 to track that.

Test Plan:
  - Set package autoreveiw to "Review".
  - Updated, got a reveiwer.
  - Set autoreview to "blocking".
  - Updated, got a blocking reviewer.

{F1311720}

{F1311721}

{F1311722}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8887, T10939

Differential Revision: https://secure.phabricator.com/D15916
2016-05-13 17:22:36 -07:00
epriestley
52ac242eb3 Implement "Auto Review" in packages with a "Subscribe" option
Summary:
Ref T10939. Ref T8887. This moves toward letting packages automatically become reviewers or blocking reviewers of owned code.

This change adds an "Auto Review" option to packages. Because adding reviewers/blocking reviewers is a little tricky, it doesn't actually have these options yet -- just a "subscribe" option. I'll do the reviewer work in the next update.

Test Plan:
Created a revision in a package with "Auto Review: Subscribe to Changes". The package got subscribed.

{F1311677}

{F1311678}

{F1311679}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8887, T10939

Differential Revision: https://secure.phabricator.com/D15915
2016-05-13 17:21:58 -07:00
epriestley
70ddb1c45f Allow packages to be added as revision reviewers via the UI
Summary:
Ref T10939. This lets you add packages as reviewers manually.

"Project Reviewers" now lists both projects and packages. I have renamed this to "Coalition Reviewers" but that's probably horrible and confusing. I'm not sure "Group Reviewers" is much better.

Test Plan:
  - Added a package as a reviewer manually.
  - Joined it, got authority over it.
  - Saw the review on my dashboard.
  - Accepted the revision, got authority extended to the package review.

{F1311652}

{F1311653}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15914
2016-05-13 17:20:09 -07:00
epriestley
3ea47d967a Allow monogrammed objects to be parsed from the arc command line in "Reviewers" and similar fields
Summary:
Ref T10939. This allows the CLI to parse reviewers and subscribers like this:

```Reviewers: epriestley, O123 Some Package Name```

The rule goes:

  - If a reviewer or subscriber starts with a monogram (like `X111`), just look that up and ignore everything until the next comma.
  - Otherwise, split it on spaces and look up each part.

This means that these are valid:

```
alincoln htaft
alincoln, htaft
#a #b epriestley
O123 Some Package, epriestley, #b
```

I think the only real downside is that this:

```
O123 Some Package epriestley
```

...ignores the "epriestley" part. However, I don't expect users to be typing package monograms manually -- they just need to be representable by `arc land` and `arc diff --edit` and such. Those flows will always add commas and make the parse unambiguous.

Test Plan:
  - Added test coverage.
  - `amend --show`'d a revision with a package subscriber (this isn't currently possible to produce using the web UI, it came from a future change) and saw `Subscribers: O123 package name, usera, userb`.
  - Updated a revision with a package subscriber.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15911
2016-05-13 17:18:35 -07:00
epriestley
44057ad269 Consider packages when calculating Differential authority
Summary:
Ref T10939. This has no effect yet since packages can not actually become reviewers, I'm just inching toward support.

  - When searching for "responsible users", include revisions that need review by packages you have authority over.
  - When calculating review authority, include authority over packages you are a member of (these currently never exist).

Test Plan:
This isn't reachable so I just `var_dump()`'d stuff and looked at the generated queries, which appeared correct/reasonable.

I'll vet this more thoroughly once packages can actually become reviewers.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15909
2016-05-13 17:17:50 -07:00
epriestley
bd9bcaa8ff Improve HTML mail rendering of inline patches
Summary: Fixes T9790. This uses a simple renderer, like the inline context renderer, that emphasizes getting a quick glance at small changes and working reasonably on mobile devices.

Test Plan:
  - Set `inline` setting to `9999`.
  - Created a diff.
  - Saw it render reasonably in HTML mail.
  - Also tested text mail to make sure I didn't break that.

{F1310137, size=full}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9790

Differential Revision: https://secure.phabricator.com/D15901
2016-05-12 12:13:40 -07:00
epriestley
cd86bf0174 Remove metamta.differential.unified-comment-context and explain it in ExtraConfigSetupCheck
Summary: Ref T10694. This setting no longer has any effect: we always show a limited amount of context now.

Test Plan: `grep`

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10694

Differential Revision: https://secure.phabricator.com/D15886
2016-05-11 06:47:58 -07:00
epriestley
412fc34557 Improve inline mail snippet rendering, possibly fixing Airmail?
Summary:
Ref T10694. General improvements:

  - Remove leading empty lines from context snippets.
  - Remove trailing empty lines from context snippets.
  - If we removed everything, render a note.
  - Try using `style` instead of `<pre>`? My thinking is that maybe Airmail has weird default rules for `<pre>`, since that's the biggest / most obvious thing that's different about this element to me.

Test Plan: Viewed normal comments locally, faked a comment on an empty line in the middle of a lot of other empty lines.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10694

Differential Revision: https://secure.phabricator.com/D15864
2016-05-06 11:58:33 -07:00
epriestley
371051ff37 Minor tweaks to pre/inline style for inline comments in HTML mail
Summary:
Ref T10694.

  - Shift margins/padding around so inlines with multiple paragraphs get reasonable spacing.
  - Add `text-decoration: none` to the "View Inline" link to kill the underline.

Test Plan: {F1265342}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10694

Differential Revision: https://secure.phabricator.com/D15863
2016-05-06 11:05:23 -07:00
epriestley
053d6111e4 Refine inline style rendering in email
Summary: Ref T10694. Move the inline style more toward a mix of standard`<pre>` style and the web UI style for inlines.

Test Plan: See screenshots in comments.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10694

Differential Revision: https://secure.phabricator.com/D15857
2016-05-06 10:34:24 -07:00
epriestley
1baef494c1 Pick context windows for inlines in a slightly smarter way
Summary:
Ref T10694. This mostly prevents us from having a degenerate case if someone leaves a 200-line inline.

  - For one-line inlines, show 1 line of context above and below (3 lines total).
  - For 3+ line inlines, show just the inline.
  - For 7+ line inlines, show only the first part.

Test Plan: Made a bunch of weird long/short/different-sized comments, saw reasonble-appearing context in text and HTML mail output.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10694

Differential Revision: https://secure.phabricator.com/D15853
2016-05-05 11:15:09 -07:00
epriestley
94c7bb605c Highlight inline diff context in HTML mail
Summary:
Ref T10694. Ref T9790. When generating inline diff context, highlight it and then mangle the highlighted output into `style="..."` so it works in HTML.

Also try to tighten up some spacing/formatting stuff.

Test Plan:
Got some output in this vein:

{F1259937}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9790, T10694

Differential Revision: https://secure.phabricator.com/D15852
2016-05-05 11:13:27 -07:00
epriestley
2025ecd3d8 Rough cut of inline comment context
Summary:
Ref T10694. This is still missing some pieces, but seems to get most of the data into the mail in a plausible format:

  - When an inline remarks on code, show the patch inline in the mail body.
  - When an inline replies to another inline, show that other inline in the mail body.
  - Apply remarkup rendering to inline content.
  - Apply basic styling to mail body blocks.

Not covered yet:

  - Syntax highlighting.
  - Diff highlighting.
  - Maybe clearer style/layout hints to connect comments to what they reply to? Current approach might get messy with inlines that have blockquotes and code blocks inside them, for example.
  - I probably want to cap the amount of diff context we ever show to ~7 lines, even if you drag over 200 lines of code.
  - CSS is a generally a bit rough still.
  - The `unified-comment-context` option is effectively always on now, and should be removed.
  - Text section is getting indented right now but probably shouldn't be.
  - Spacing, etc., might be a bit off.

Test Plan:
Rigged Home to render these things, got a plausible-looking render (top is text, bottom is HTML):

{F1259052}

Sent myself some inline comment mail, got a plausible result.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10694

Differential Revision: https://secure.phabricator.com/D15850
2016-05-05 09:23:08 -07:00
Chad Little
562d427f09 New icons, colors for per reviewer status
Summary: Fixes T10906, Fixes T10820. Adds new icons, grey-er colors for previous states. Also, I think fixed a few bugs?

Test Plan: Fake each state, verify icon is as intended.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10820, T10906

Differential Revision: https://secure.phabricator.com/D15830
2016-05-01 12:52:40 -07:00
Aviv Eyal
1344dda756 Parse Tags in commits message for revisions
Summary: This will stop breaking if you have subscribers and tags when updating a revision (`Error parsing field "Subscribers": The objects you have listed include objects which do not exist (Tags:)`), which I broke in D15749.

Test Plan: run through arc-diff --update that failed earlier.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15762
2016-04-20 01:46:17 +00:00
Eitan Adler
c9daa2b0ad Consistently refer to 'Projects' as 'Tags'
Summary:
In calendar, dashboard, diffusion, diviner, feed, fund,
maniphest, pholio, ponder, and slowvote use the term 'tags' if possible.

This intenctionally skips diffusion, differential, and the projects application itself.

Ref T10326 Ref T10349

Test Plan: inspection on a running, locally modified, system

Reviewers: avivey, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T10835, T10326, T10349

Differential Revision: https://secure.phabricator.com/D15753
2016-04-19 16:48:21 +00:00
Aviv Eyal
091a64e91b Rename Differential field Projects to Tags
Summary: Users can't find the "Tags" field in the Edit Menu; Added keyword "Tag".

Test Plan: Looked in Edit page; I think this shouldn't change anything else?

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15749
2016-04-18 19:33:40 +00:00
Povilas Balzaravicius Pawka
f05c3e41b9 Fixed localcommits include on getDiffDict
Summary: Ref T10808

Test Plan: Call `differential.querydiffs` method and expect 'local:commits' property be added to the result.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T10808

Differential Revision: https://secure.phabricator.com/D15710
2016-04-15 18:06:32 +03:00
Chad Little
437ff2a718 Normalize case on active operations
Summary: Found another bouncing around.

Test Plan: Review in diff

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15651
2016-04-06 17:33:59 -07:00
Chad Little
8f67d59d28 Bump font size on property headers
Summary: Bumps to 14px, fixes some on Differential

Test Plan: view various headers in Differential

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15647
2016-04-06 23:08:20 +00:00
Chad Little
8aad862cd4 Normalize casing on property boxes
Summary: Going to render these all normal case instead of all caps, and bump up the font size. Should be more consistent. Yellow if you green anything orange.

Test Plan: grep, lint

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15645
2016-04-06 15:33:15 -07:00
epriestley
61301ead90 Fix javascript interaction with right-hand-side images in two-up diff views in Differential
Summary: Fixes T10704. This is just bad copy-paste -- "O" for "old" should be "N" for "new".

Test Plan:
  - Followed steps on T10704.
  - Applied patch.
  - Marked inline done, replied, etc. No more JS errors.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10704

Differential Revision: https://secure.phabricator.com/D15566
2016-04-01 12:54:43 -07:00
Chad Little
d9bb66f610 Update Differential edit pages to new UI
Summary: Updates using PHUITwoColumnView, new headers, etc.

Test Plan: New Diff, Update Diff, View Standalone pages, Edit pages.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15549
2016-03-30 12:45:59 -07:00
epriestley
601aaa5a86 Modularize content sources
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.

Split ContentSource apart so applications can add new content sources.

Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.

- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.

{F1190182}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10537

Differential Revision: https://secure.phabricator.com/D15521
2016-03-26 11:59:45 -07:00
Chad Little
d76652b331 Update Harbormaster for two column
Summary: Updates the Harbormaster UI to match the new two column everywhere else.

Test Plan: Did best I could, tested builds, plans, steps, buildables. Unable to test lint/unit locally, I need to set that up. Kick the tires for me pls. :3

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15523
2016-03-25 21:41:47 +00:00
epriestley
f82db7524b Add a "Build with CircleCI" build step
Summary: Ref T9456. Some rough edges and we can't complete the build yet since I haven't written a webhook, but this mostly seems to be working.

Test Plan:
  - Ran this build on some stuff.
  - Ran a normal HTTP step build to make sure I didn't break that.

{F880301}

{F880302}

{F880303}

Reviewers: chad

Reviewed By: chad

Subscribers: JustinTulloss, joshma

Maniphest Tasks: T9456

Differential Revision: https://secure.phabricator.com/D14286
2016-03-22 12:12:11 -07:00
Chad Little
8f94aa8a06 Update Diffusion UI
Summary: This updates (all?) of Diffusion/Audit to new UI, included edit and other extra form pages. It's fairly complete but I don't know all the nooks and crannies so to speak to fully verify I didn't mess anything up.

Test Plan: Tested creating new repositories, browsing, searching, auditing. Need more eyes.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15487
2016-03-17 12:05:14 -07:00
epriestley
121e68e3ad Fix an issue with rendering unit messages for diffs with no buildable
Summary: Fixes T10591. This was accidentally reverted in 148a50e48b, probably when resolvign a merge/rebase.

Test Plan: Will push to production.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10591

Differential Revision: https://secure.phabricator.com/D15474
2016-03-15 08:19:24 -07:00
Chad Little
d76175285e Update Diff view page to new layout
Summary: Converts Diff View, single column though.

Test Plan: Upload a new diff, review page.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15470
2016-03-13 16:04:38 -07:00
Chad Little
148a50e48b Convert Differential to new layout
Summary:
First pass at converting Differential, I likely have some buggy-poos but thought I'd toss this up now in case very bad bugs present.

To do:
- Need to put status back on Hovercards
- "Diff Detail" probably needs a better design

Test Plan: Looking at lots of diffs, admittedly I dont have harbormaster, etc, running locally. Checked Diffusion for Table of Content changes on small and large commits.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15463
2016-03-12 13:04:21 -08:00
Burak Yigit Kaya
60b42750b6 Cast old duration values for unit tests to float in DifferentialController
Summary: Fixes T10549.

Test Plan: N/A

Reviewers: #blessed_reviewers, avivey, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T10549

Differential Revision: https://secure.phabricator.com/D15452
2016-03-10 18:15:46 -08:00
epriestley
68b468a846 Partially improve threading UI for adjacent inline comments
Summary:
Ref T10563. This isn't a complete fix, but should make viewing complex inline threads a little more manageable.

This just tries to put stuff in thread order instead of in pure chronological order. We can likely improve the display treatment -- this is a pretty minimal approach, but should improve clarity.

Test Plan:
T10563 has a "before" shot. Here's the "after":

{F1169018}

This makes it a bit easier to follow the conversations.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10563

Differential Revision: https://secure.phabricator.com/D15459
2016-03-10 17:40:13 -08:00
epriestley
7b8da99914 Move DifferentialRevisionViewController to newPage()
Summary: I think this works?

Test Plan:
i am wizard

{F1168808}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15457
2016-03-10 13:22:25 -08:00
epriestley
2ddd78647b Improve "Land Revision" errors for issues related to staging areas
Summary: Ref T10093. Changes must be pushed to staging before they can be landed from the web.

Test Plan:
Changes must be pushed to staging before they can be landed from the web.

{F1161909}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10093

Differential Revision: https://secure.phabricator.com/D15427
2016-03-07 12:46:07 -08:00
epriestley
aaab1011e5 Give AphrontTagView a getViewer(), deprecate getUser()
Summary:
Two minor changes here:

  - Replace `get/setUser()` with `get/setViewer()` for consistency with everything else.
  - `getViewer()` now throws if no viewer is set. We had a lot of code that either "should" check this but didn't, or did check it in an identical way, duplicating work. In contrast, very little code checks for a viewer but works if one is not present.

Test Plan:
  - Grepped for `->user`.
  - Attempted to fix all callsites inside `*View` classes.
  - Browsed around a bunch of applications, particularly Calendar, Differential and Diffusion, which seemed most heavily affected.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15412
2016-03-06 09:27:38 -08:00
epriestley
abb4c03b47 Remove shouldShowSubscribersProperty() from SubscribableInterface
Summary:
Every caller returns `true`. This was added a long time ago for Projects, but projects are no longer subscribable.

I don't anticipate needing this in the future.

Test Plan: Grepped for this method.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15409
2016-03-06 06:01:36 -08:00
Chad Little
2b1ac4fcec Update Maniphest for PHUITwoColumnView
Summary: Reworks Maniphest into a two column view. Moves priority and color to header, assignee to sidebar. quest points to header, and author to gutter. may be some confusion since priority only displays on open tickets.

Test Plan: with and without description, custom fields, points, tablet, mobile and desktop.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15396
2016-03-04 17:26:38 -08:00
epriestley
181e030535 Give unit test results their own table in Differential
Summary: Ref T10457. This gives unit test results a more first-class treatment in the Differential UI, and consolidates some rendering code.

Test Plan:
Before:

{F1135536}

After:

{F1135537}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10457

Differential Revision: https://secure.phabricator.com/D15365
2016-02-29 14:27:22 -08:00
epriestley
3c19b72ca0 Begin making Harbormaster unit test results a little easier to read
Summary: Ref T10457. These lack color and iconography and are difficult to parse. Make them easier to read.

Test Plan:
Before:

{F1135396}

After:

{F1135399}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10457

Differential Revision: https://secure.phabricator.com/D15362
2016-02-29 14:24:08 -08:00
epriestley
fdca684814 Slightly improve Buildable list in Harbormaster
Summary:
Ref T10457. This makes diffs/revisions show the revision as the buildable title, and commits show the commit as the title.

Previously, the title was "Buildable X".

Also makes icons/colors/labels more consitent.

Test Plan: {F1131885}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10457

Differential Revision: https://secure.phabricator.com/D15355
2016-02-27 07:11:30 -08:00
Chad Little
f35509e30e Update to use PHUIRemarkupView everywhere possible
Summary: Moves all the one off object calls to PHUIRemarkupView, adds a "Document" call as well (future plans).

Test Plan: Visited most pages I could get access to, but may want extra careful eyes on this diff.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15281
2016-02-16 14:05:53 -08:00
Chad Little
6bb24e1d0c Move PhabricatorHovercard to PHUIHovercard
Summary: No UI changes, just some search and replace for UI consistency.

Test Plan: Test person and object hovercards still work. UIExamples too.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15172
2016-02-03 16:26:30 +00:00
Aviv Eyal
8f0d9c3295 Remove email prefixes from doorkeeper titles
Summary: Fixes T10176. The prefix is not useful in the JIRA context, and doubtfully useful in Asana.

Test Plan: Load, make comment on revision, see link in JIRA is pretty.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T10176

Differential Revision: https://secure.phabricator.com/D15119
2016-01-28 18:48:28 +00:00
Chad Little
fe5cd4ca2c Move FontIcon calls to Icon
Summary: Normalizes all `setFontIcon` calls to `setIcon`.

Test Plan: UIExamples, Almanac, Apps list, etc.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, hach-que, yelirekim

Differential Revision: https://secure.phabricator.com/D15129
2016-01-28 08:48:45 -08:00
Chad Little
36158dbdc0 Convert all calls to 'IconFont' to just 'Icon'
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.

Test Plan: tested uiexamples, lots of other random pages.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15125
2016-01-27 20:59:27 -08:00
Mike Riley
e7195628d5 Use correct transaction types when creating diffs
Summary: See T10214 for context.  These transaction types are obviously wrong as far as I can tell.

Test Plan:
Created a revision and didn't see an error in the daemon log.

```lang=php
<?php

require_once dirname(__FILE__).'/phabricator/scripts/__init_script__.php';

$yelirekim = (new PhabricatorPeopleQuery)
  ->setViewer(PhabricatorUser::getOmnipotentUser())
  ->withUsernames(['yelirekim'])
  ->executeOne();

$raw_diff = (new PhabricatorDifferenceEngine)
  ->generateRawDiffFromFileContent('oldfile', 'newfile');
$diff = (new ConduitCall('differential.createrawdiff', [
      'diff' => $raw_diff,
    ]))
  ->setUser($yelirekim)
  ->execute();

$xactions = (new DifferentialDiffTransactionQuery)
  ->setViewer($yelirekim)
  ->withObjectPHIDs([$diff['phid']])
  ->execute();

foreach ($xactions as $xaction) {
  echo $xaction->getPHID().':'.$xaction->getTitle().PHP_EOL;
}
```

for sanity

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: michaeljs1990, epriestley

Differential Revision: https://secure.phabricator.com/D15112
2016-01-25 02:17:42 -08:00
epriestley
5c2e49a812 Allow any user to watch any project they can see
Summary:
Ref T6183. Ref T10054. Historically, only members could watch projects because there were some weird special cases with policies. These policy issues have been resolved and Herald is generally powerful enough to do equivalent watches on most objects anyway.

Also puts a "Watch Project" button on the feed panel to make the behavior and meaning more obvious.

Test Plan:
  - Watched a project I was not a member of.
  - Clicked the feed watch/unwatch button.

{F1064909}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6183, T10054

Differential Revision: https://secure.phabricator.com/D15063
2016-01-19 19:38:30 -08:00
epriestley
45a4a68628 Distinguish between "no coverage information" and "no test coverage" better
Summary: Fixes T10169. Diffs with no build targets were incorrectly showing as though they had no test coverage, when we actually want to show them having no coverage information available.

Test Plan: Viewed an older revision, saw a column of "Not Executable" before change, now see no column.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10169

Differential Revision: https://secure.phabricator.com/D15044
2016-01-18 08:34:03 -08:00
epriestley
fdd5500fec Change wording of "Show Entire File" to "Show All Context"
Summary: Fixes T10139. This clarification seems reasonable to me.

Test Plan:
  - Viewed a revision with context.
  - Clicked "Show All Context".
  - Saw "All Context Shown".

{F1060624}

{F1060625}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10139

Differential Revision: https://secure.phabricator.com/D15009
2016-01-13 11:09:01 -08:00
epriestley
efba69a440 Give users an explicit error if they try to upload a too-large diff to Differential
Summary:
Ref T8612. If a change affects more than 10K paths + hunks, tell the user it's too big and don't bother trying to write it. We're mostly bounded by INSERTs here.

Also, fix an issue with file upload errors. The keys are real PHP constants, but were accidentally converted to strings in D12797, causing every error to show as "unknown error".

Test Plan: {F1057509}

Reviewers: joshuaspence

Reviewed By: joshuaspence

Maniphest Tasks: T8612

Differential Revision: https://secure.phabricator.com/D14977
2016-01-08 18:53:33 -08:00
epriestley
d1fb2f7fb9 Make diffusion.filecontentquery return file PHIDs instead of raw content
Summary:
Fixes T9319. Proxied requests (e.g., in the cluster) for binary files (like images) currently fail because we can not return binary data over Conduit in JSON.

Although Conduit will eventually support binary-safe encodings, a cleaner approach to this is just to return a `filePHID` instead of the raw content. This is generally faster and more flexible, and gives us more opportunities to add caching later.

After making the call, the client pulls the file data separately.

We also no longer need to return a complex data structure because we don't do blame over this call any longer.

Test Plan:
  - Viewed images in Diffusion.
  - Viewed READMEs in Diffusion.
  - Used `bin/differential attach-commit rX Dy` to hit attach pathway.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9319

Differential Revision: https://secure.phabricator.com/D14970
2016-01-08 09:29:16 -08:00
epriestley
da3963b009 Convert a low-level VCS query in Diff extraction to a Conduit call
Summary: Ref T9319. Ref T2783. This won't currently work in a future environment where daemons and repositories are not on the same host. Send it over Conduit instead.

Test Plan: Used `bin/differential attach-commit rX Dy` to force attachment, saw valid content pull over Conduit.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2783, T9319

Differential Revision: https://secure.phabricator.com/D14969
2016-01-08 09:28:15 -08:00
epriestley
413ca12fda Move commit attachment to a separate CLI command
Summary:
Ref T9319. See D14967. As before, this is making a deeply-buried, complex operation easier to test by providing a CLI command.

This adds `bin/differential attach-commit rXnnnn Dnnnn` to pretend that `rXnnnn` was just committed and matched `Dnnnn`.

Test Plan:
  - Ran `bin/differential attach-commit X Y` for several different values, saw updates in the UI.
  - Faked the message parser to make sure stuff still worked there.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9319

Differential Revision: https://secure.phabricator.com/D14968
2016-01-08 09:27:57 -08:00
epriestley
0dd947cced Move diff extraction from commits to a separate test with a CLI command
Summary:
Ref T9319. When we discover a commit, we sometimes update the corresponding revision with a "this is the actual committed change" diff and send out a link to the changes between review and commit.

This is currently very difficult to test, because it only happens the first time and you have to either go set up a bunch of objects or add a bunch of special casing to the parser to hit the workflow.

I'm making some changes to how it pulls file content. To make those changes easier to test, first start extracting this stuff so the code can be run with `bin/differential extract ...` instead of needing to do a bunch of more complicated setup steps.

Test Plan:
  - Ran `bin/differential extract ...` to extract diffs from commits.
  - Forced my way through the daemon workflow by faking out a bunch of flags, got a clean extract + attach + update. After this patch, this should rarely be necessary.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9319

Differential Revision: https://secure.phabricator.com/D14967
2016-01-08 09:22:37 -08:00
epriestley
9febfb26a0 Convert diffusion.looksoon to use repository identifiers instead of callsigns
Summary:
Ref T4245. Like everything else, accept more identifiers.

This needs a change in `arc`, which I've made a note about elsewhere.

Test Plan: Used "Update Now" from web UI, saw update get scheduled.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14932
2016-01-04 06:54:20 -08:00
epriestley
be5b89687e Separate external editor integration from callsigns
Summary: Ref T4245. Pass the whole repository in so it can do something else in a future change.

Test Plan: Loaded changesets in Diffusion.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14931
2016-01-04 06:54:01 -08:00
epriestley
367955f3fd Improve UX and messaging for certain errors when landing revisions
Summary:
Ref T9994.

  - Allow errors to be dismissed.
  - Tailor messaging for closed/abandoned revisions.
  - Reduce scare messaging on land dialog, since it's not really that scary anymore.

Test Plan:
  - Dismissed errors.
  - Hit new warnings.
  - Wasn't as scared when landing.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9994

Differential Revision: https://secure.phabricator.com/D14886
2015-12-27 08:01:20 -08:00
epriestley
bdc517485c Modernize Hovercard implementation
Summary:
Ref T8980. Move away from events to EngineExtensions.

This also simplifies hovercards a bit:

  - Removes tasks from revision cards.
  - Removes blockers/blocked from task cards.
  - Removes "Send Message" from user cards.

These mostly felt cluttery to me. Open to arguments to retain them. I think we can make better use of the space, though (e.g., flags, projects + board columns).

Test Plan:
  - Viewed people, task, revision, commit and project hovercards.

{F1043256}

{F1043257}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8980

Differential Revision: https://secure.phabricator.com/D14878
2015-12-24 12:18:28 -08:00
epriestley
3ec07c4987 Show hovercards for most links in object property views
Summary:
Ref T8980. This isn't 100% coverage but should be pretty much all of the common ones.

These feel a touch iffy to me at first glance so I didn't go crazy trying to hunt all of them down. I have some other plans for them so maybe they'll feel better by the end of it.

Test Plan: Hovered over author, reviewers, blocked tasks, projects, etc.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8980

Differential Revision: https://secure.phabricator.com/D14877
2015-12-24 12:10:56 -08:00
epriestley
992dedcadd Modernize Differential SearchEngine just enough to get NUX
Summary: Ref T10032. This is sufficent to hit NUX without doing anything bad.

Test Plan:
  - Visited NUX.
  - Browsed normally.

{F1043191}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10032

Differential Revision: https://secure.phabricator.com/D14876
2015-12-24 10:47:52 -08:00
epriestley
19b2eb57a9 Improve lipsum generation of projects
Summary:
Fixes T9156.

  - Fix hashtag generation.
  - Fix various badnesses.
  - Improve project name generator.

Test Plan:
```
$ ./bin/lipsum generate projects
 GENERATORS  Selected generators: Projects.
 WARNING  This command generates synthetic test data, including user accounts. It is intended for use in development environments so you can test features more easily. There is no easy way to delete this data or undo the effects of this command. If you run it in a production environment, it will pollute your data with large amounts of meaningless garbage that you can not get rid of.

    Are you sure you want to generate piles of garbage? [y/N] y

 LIPSUM  Generating synthetic test objects forever. Use ^C to stop when satisfied.
Generated "Project": Self-Flying Data Center Swag Performance
Generated "Project": Optimize Cars
Generated "Project": Triaging Culture Optimization
Generated "Project": Automating Experience
Generated "Project": Accelerating NUX Performance
Generated "Project": Optimizing Culture Optimization
Generated "Project": Optimize Hardware
```

{F1042949}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9156

Differential Revision: https://secure.phabricator.com/D14874
2015-12-24 09:06:56 -08:00
epriestley
ba37149bf9 Improve bin/lipsum UX
Summary: Ref T9156. This makes the UX a little more modern/standard/safe.

Test Plan:
```
epriestley@orbital ~/dev/phabricator $ ./bin/lipsum generate
Choose which type or types of test data you want to generate, or select "all".

      - Differential Revisions
      - Files
      - Maniphest Tasks
      - Pastes
      - Pholio Mocks
      - Projects
      - User Accounts
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9156

Differential Revision: https://secure.phabricator.com/D14873
2015-12-24 09:06:35 -08:00
epriestley
99c9df96b4 Convert all "DocumentIndexers" into "FulltextEngines"
Summary: Ref T9979. This simplifies/standardizes the code a bit, but mostly gives us more consistent class names and structure.

Test Plan:
  - Used `bin/search index --type ...` to index documents of every indexable type.
  - Searched for documents by unique text, found them.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9979

Differential Revision: https://secure.phabricator.com/D14842
2015-12-21 17:25:23 -08:00
epriestley
ecc3314a25 Modularize transaction/comment indexing in the FulltextEngine
Summary:
Ref T9979. This is currently hard-coded but can be done in a generic way.

This has one minor behavioral changes: answer text is no longer included in the question text index in Ponder. I'm not planning to accommodate that for now since I don't want to dig this hole any deeper than I already have. This behavior should be different anyway (e.g., index the answer, then show the question in the results or something).

Test Plan:
  - Put a unique word in a Maniphest comment.
  - Searched for the word.
  - Found the task.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9979

Differential Revision: https://secure.phabricator.com/D14837
2015-12-21 17:24:40 -08:00
Chad Little
5fecd55d6e More NUX states
Summary: Ref T10032, adds "Basic" NUX to more applications.

Test Plan: Visit each with ?nux=true and click on the create link. T10032 is tracking which apps need general modernization to pick up these changes.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10032

Differential Revision: https://secure.phabricator.com/D14847
2015-12-21 13:13:44 -08:00
epriestley
8a81d208c4 Don't show "master (branched from master)" in UI
Summary:
Ref T3462. If someone works directly on `master`, we currently show "Branch: master (branched from master)" in the UI.

Although this is sort of technically accurate, it is confusing.

Instead, just show "Branch: master" in this situation.

Test Plan: Saw "master" instead of "master (branched from master)".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T3462

Differential Revision: https://secure.phabricator.com/D14829
2015-12-19 12:57:59 -08:00
epriestley
2868a69f65 Remove all setObjectURI() from ActionListViews
Summary:
Ref T10004. After D14804, we get this behavior by default and no longer need to set it explicitly.

(If some endpoint did eventually need to set it explicitly, it could just change what it passes to `setHref()`, but I believe we currently have no such endpoints and do not foresee ever having any.)

Test Plan:
  - As a logged out user, clicked various links in Differential, Maniphest, Files, etc., always got redirected to a sensible place after login.
  - Grepped for `setObjectURI()`, `getObjectURI()` (there are a few remaining callsites, but to a different method with the same name in Doorkeeper).

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T10004

Differential Revision: https://secure.phabricator.com/D14805
2015-12-17 08:30:22 -08:00
epriestley
954cd4b1b6 When arc has sent target branch data up after D14736, use it in the UI and "Land Revision"
Summary:
Ref T9952. Ref T3462. After D14736, if we have information about the target/"onto" branch, use it in the UI:

  - Show "feature (branched from master)" instead of "feature".
  - Default "Land Revision" to hit the correct branch.

Test Plan:
  - Branched from `test` with branch tracking.
  - Diffed.
  - Saw "feature (branched from test)" in UI.
  - Saw "test" fill as default in "Land Revision", despite the repository having a different default branch.

{F1020587}

{F1020588}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T3462, T9952

Differential Revision: https://secure.phabricator.com/D14737
2015-12-10 15:24:38 -08:00
epriestley
f60548d081 Default "Land Revision" dialog to land into the default branch for the repository
Summary:
Ref T9952. Default the branch target in the dialog to be whatever branch is the default branch for the repository.

This will be correct for repositories like ours (which land everything into `master`) and correct most of the time for repositories which have some other "primary" branch (maybe `development`).

It won't be great if there are multiple open lines of development in a repository (for example, some changes go to `newdesignpro` and some changes go to `legacy-1.2`). I'll do work in T3462 next to improve those cases so we can pick a better default.

Test Plan:
  - Saw dialog default to "master".
  - Changed repo default branch, saw it default to "notmaster" instead.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9952

Differential Revision: https://secure.phabricator.com/D14734
2015-12-10 14:21:59 -08:00
epriestley
856bdaf77e Add an "Onto Branch" selector control to "Land Revision" dialog
Summary:
Ref T9952. This adds a typeahead so you can pick a branch to target.

It does not choose a default branch, the user must pick a branch explicitly.

Test Plan:
  - Landed rGITTESTd587fada48fc to `master` (by typing "master").
  - Landed rGITTEST86c339b2ef01 to `notmaster` (by typing "notmaster").

{F1020531}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9952

Differential Revision: https://secure.phabricator.com/D14733
2015-12-10 14:21:38 -08:00
epriestley
618cec23d8 Make notification counts properly translatable
Summary:
Ref T9132. When I've touched `PhabricatorApplication` I keep hitting this bad `pht()` junk.

The warning is correct, these strings are not extactable and can not be translated.

Fix it so they can be extracted and translated.

Broadly, in all cases we want to render one of these:

> 95 Things (for fewer than some limit)
> 99+ Things (when we hit the limit)

Test Plan: Looked at homepage status counts, moused over them, saw reasonable strings. Grepped for removed method.

Reviewers: chad

Reviewed By: chad

Subscribers: joshuaspence

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14638
2015-12-03 07:06:39 -08:00
epriestley
0d01dab5a3 Partially revert D14511 to fix "INLINE COMMENTS" in mail
Summary:
Ref T9845. In Differential, this is not a remarkup block -- it's a mail section. `addTextSection()` has special magic behavior when handed a prebuilt section since D9375.

Swapping to `addRemarkupSection()` causes the error in T9845 and renders nothing in the comment section.

Even if it were a block of text, it would not be appropriate to add it as remarkup. This would incorrectly render comments in files like `__init__.py`, which are common on Python (the filename would render as "__init__.py"). Okay that's a bad example since it works fine but, uh, a file named `T123` would be no good or whatever.

I'll realign T9845 to clean this up and fix it more durably.

Test Plan: Sent myself some mail with inline comments, saw them in the mail.

Reviewers: joshuaspence, chad

Reviewed By: chad

Maniphest Tasks: T9845

Differential Revision: https://secure.phabricator.com/D14589
2015-11-28 13:40:57 -08:00
Joshua Spence
2047483cc0 Render Remarkup in emails
Summary: Ref T992. I noticed that `ManiphestTask` mail doesn't render Remarkup properly (instead, it renders Remarkup literally). I //think// this is because the code calls `addTextSection()` rather than `addRemarkupSection()`.

Test Plan: Created a new Maniphest Task and saw Remarkup in the generated self-email (inspect the email contents with `./bin/mail show-outbound`). I didn't test the other affected applications.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T992

Differential Revision: https://secure.phabricator.com/D14511
2015-11-24 06:43:01 +11:00
epriestley
5aae89babb Fix file PHID extraction in Owners and Differential
Summary:
Ref T9787. To fix this, I want to change how file PHIDs are extracted slightly: specifically, I'm going to extract them later in the editing process.

Before doing this, clean up a couple of bad implementations:

  - Owners extracts its description as a file PHID. This is an error.
    - Extract the description as a remarkup block instead.
    - Add an edge table so stuff like file attachment works properly.
  - Differential has a no-op extract method. This is presumably just a copy/paste issue from long ago.

Test Plan:
  - Edited a revision in Differential.
  - Dropped a file into the description of an Owners package.
    - Before change: this did not attach the file.
    - After change: the file now attaches properly and shows up as "Attached" in the file details.

Reviewers: chad, joshuaspence

Reviewed By: joshuaspence

Subscribers: joshuaspence

Maniphest Tasks: T9787

Differential Revision: https://secure.phabricator.com/D14493
2015-11-17 08:36:50 -08:00
Joshua Spence
1f1c3f4075 Allow lint messages to be rendered as Remarkup
Summary: Some linter messages, such as those produced by `ArcanistPHPCompatibilityXHPASTLinterRule`, contain backticks but are currently rendered as Remarkup literals. I think that it is generally desirable to allow lint messages to be rendered as Remarkup, although we should ideally have a way to render Remarkup for use on the command line (I actually think that this already exists, but I don't think that `arc lint` does this when rendering linter messages).

Test Plan: Resubmitted D14481 to my dev install and saw Remarkuped lint messages.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14485
2015-11-15 19:50:10 +00:00
Joshua Spence
c35b564f4d Various translation improvements
Summary: Depends on D14070.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D14073
2015-11-03 07:02:46 +11:00
Joshua Spence
495cb7a2e0 Mark PhabricatorPHIDType::getPHIDTypeApplicationClass() as abstract
Summary: Fixes T9625. As explained in a `TODO` comment, seems reasonable enough.

Test Plan: Unit tests.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, hach-que

Maniphest Tasks: T9625

Differential Revision: https://secure.phabricator.com/D14068
2015-11-03 06:47:12 +11:00
epriestley
2c3dbc48ee Move "Next Step" to a custom field in Differential
Summary:
Fixes T9672. This was never turned into a custom field, for no particular reason. Convert it into one.

This is substantially similar to the existing "Apply Patch" field, which does the same thing (only shows a command).

We might rethink or remove this eventually (e.g., in a post-"Land Revision" world) but this makes it easier, at the very least.

Test Plan:
  - Viewed a non-accepted revision (no hint).
  - Viewed an accepted revision from a raw diff source (no hint).
  - Viewed an accepted revision from Git (`arc land` hint).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9672

Differential Revision: https://secure.phabricator.com/D14367
2015-10-30 16:00:53 +00:00
epriestley
1c7443f8f2 Make "Land Revision" button state consistent, prevent non-accepted lands
Summary:
Ref T182. Make the disabled state of the button more accurately reflect whether clicking it will work.

Don't allow "land" to proceed unless the revision is accepted.

Test Plan: Saw button in disabled state, clicked it, got "only accepted revisions" message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14350
2015-10-27 18:51:59 +00:00
epriestley
cea633f698 Don't show error operations after a successful land operation
Summary:
Ref T182. When viewing a revision, if there are several error operations and then a success operation, we currently show the last error. This is misleading.

Instead, don't show anything if there's a success (this may require tuning eventually if you can land multiple times onto different branches or whatever, but should be reasonable for now).

Also make the table a little nicer, particularly for merge failure output.

Test Plan: {F910385}

Reviewers: chad, Mnkras

Reviewed By: Mnkras

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14348
2015-10-26 21:27:04 +00:00
epriestley
a0fba642b3 Show the oldest non-failing revision land operation, or the newest failure
Summary:
Ref T182.

  - We just show the oldest operation right now, but we usually care about the oldest non-failure.
  - Only query for actual land operations when rendering the revision operations dialog (maybe eventually we'll show more stuff?).
  - For now, prevent multiple lands / repeated lands or queueing up lands while other lands are happening.

Test Plan: Landed a revision. Tried to land it more / again.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14338
2015-10-26 19:58:37 +00:00
epriestley
5b619862cb Show a more reasonable status element for pull requests
Summary:
Ref T182. Replace the total mess we had before with a sort-of-reasonable element.

This automatically updates using "javascript".

Test Plan:
{F901983}

{F901984}

Used "Land Revision", saw the land status go from "Waiting" -> "Working" -> "Landed" without having to mash reload over and over again.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14314
2015-10-21 11:28:26 -07:00
epriestley
f3f3d95702 When landing revisions via repository automation, use better metadata
Summary: Ref T182. Make a reasonable attempt to get the commit message, author, and committer data correct.

Test Plan: BEHOLD: rGITTEST810b7f17cd0c909256a45d29a5062fcf417d0489

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14280
2015-10-14 10:50:53 -07:00
epriestley
43bee4562c If the stars align, make "Land Revision" kind of work
Summary:
Ref T182. If 35 other things are configured completely correctly, make it remotely possible that this button may do something approximating the thing that the user wanted.

This primarily fleshes out the idea that "operations" (like landing, merging or cherry-picking) can have some beahavior, and when we run an operation we do whatever that behavior is instead of just running `git show`.

Broadly, this isn't too terrible because Drydock seems like it actually works properly for the most part (???!?!).

Test Plan: {F876431}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14270
2015-10-13 15:46:30 -07:00
epriestley
b4af57ec51 Rough cut of DrydockRepositoryOperation
Summary:
Ref T182. This doesn't do anything interesting yet and is mostly scaffolding, but here's roughly the workflow. From previous revision, you can configure "Repository Automation" for a repository:

{F875741}

If it's configured, a new "Land Revision" button shows up:

{F875743}

Once you click it you get a big warning dialog that it won't work, and then this shows up at the top of the revision (completely temporary/placeholder UI, some day a nice progress bar or whatever):

{F875747}

If you're lucky, the operation eventually sort of works:

{F875750}

It only runs `git show` right now, doesn't actually do any writes or anything.

Test Plan:
  - Clicked "Land Revision".
  - Watched `phd debug task`.
  - Saw it log `git show` to output.
  - Verified operation success in UI (by fiddling URL, no way to get there normally yet).

Reviewers: chad

Reviewed By: chad

Subscribers: revi

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14266
2015-10-13 15:46:12 -07:00
Chad Little
bb37ad65a2 Update Differential for handleRequest
Summary: Moves from processRequest to handleRequest.

Test Plan: New diff, edit diff, leave comment, view list, browse revisions, etc.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14252
2015-10-11 08:18:42 -07:00
epriestley
9c798e5cca Provide bin/garbage for interacting with garbage collection
Summary:
Fixes T9494. This:

  - Removes all the random GC.x.y.z config.
  - Puts it all in one place that's locked and which you use `bin/garbage set-policy ...` to adjust.
  - Makes every TTL-based GC configurable.
  - Simplifies the code in the actual GCs.

Test Plan:
  - Ran `bin/garbage collect` to collect some garbage, until it stopped collecting.
  - Ran `bin/garbage set-policy ...` to shorten policy. Saw change in web UI. Ran `bin/garbage collect` again and saw it collect more garbage.
  - Set policy to indefinite and saw it not collect garabge.
  - Set policy to default and saw it reflected in web UI / `collect`.
  - Ran `bin/phd debug trigger` and saw all GCs fire with reasonable looking queries.
  - Read new docs.

{F857928}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9494

Differential Revision: https://secure.phabricator.com/D14219
2015-10-02 09:17:24 -07:00
epriestley
2728a9f964 Allow builds to have parameters
Summary: Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers).

Test Plan: With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters.

Reviewers: chad, hach-que

Reviewed By: hach-que

Maniphest Tasks: T9352

Differential Revision: https://secure.phabricator.com/D14222
2015-10-02 06:32:08 -07:00
epriestley
878a493301 Begin standardizing garbage collectors
Summary: Ref T9494. Improve support infrastructure for garbage collectors.

Test Plan:
  - Ran `bin/phd debug trigger`, saw collectors execute.

{F857852}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9494

Differential Revision: https://secure.phabricator.com/D14218
2015-10-01 16:58:43 -07:00
epriestley
4496176924 Add staging area support to Harbormaster/Drydock + various fixes
Summary:
Ref T9252. This primarily allows Harbormaster to request (and Drydock to fulfill) working copies with a patch from a staging area. Doing this means we can do builds on in-review changes from `arc diff`.

This is a little cobbled-together but should basically work.

Also fix some other issues:

  - Yielded, awakend workers are fine to update but could complain.
  - We can't log slot lock failures to resources if we don't end up saving them.
  - Killing the transaction would wipe out the log.
  - Fix some TODOs, etc.

Test Plan: Ran Harbormaster builds on a local revision.

Reviewers: hach-que, chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14214
2015-10-01 16:55:01 -07:00
Ray Lillywhite
1ac919c29c Improved example setting for differential.generated-paths
Summary: This is to make it more obvious how to ignore a folder that may be at the root, and it resolves https://secure.phabricator.com/T8894

Test Plan: Just a documentation change

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, nornagon

Differential Revision: https://secure.phabricator.com/D14193
2015-09-29 14:14:31 -07:00
epriestley
284fe0fe51 Allow Harbormaster to lease working copies from Drydock
Summary: Ref T9252. This is still crude in a few ways but basically works, at least for commits.

Test Plan:
  - Made a build plan with just this build step.
  - Ran `bin/harbormaster build --plan 10 ...` on a commit.
  - It actually built a working copy, leased it, took no action, and released the lease. MAGIC~~~

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14160
2015-09-24 17:29:47 -07:00
epriestley
1c45a7d8e2 Revert "Allow search results to be snippeted, roughly"
Summary:
This reverts commit 1583738842.

See T8646 for discussion. This version of the feature feels terrible on real data.

Test Plan: Strict revert.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14097
2015-09-10 20:57:26 -07:00
epriestley
4e181a5611 Clean up browse/history links in Diffusion
Summary:
Fixes T9126. In particular:

  - Add "Browse" links to all history views.
  - Use icons to show "Browse" and "History" links, instead of text.
  - Use FontAwesome.
  - Generally standardize handling of these elements.

This might need a little design attention, but I think it's an improvement overall.

Test Plan:
  - Viewed repository history.
  - Viewed branch history.
  - Viewed file history.
  - Viewed table of contents on a commit.
  - Viewed merged changes on a merge commit.
  - Viewed a directory containing an external.
  - Viewed a deleted file.

{F788419}

{F788420}

{F788421}

{F788422}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9126

Differential Revision: https://secure.phabricator.com/D14096
2015-09-10 19:28:49 -07:00
epriestley
1583738842 Allow search results to be snippeted, roughly
Summary:
Ref T8646. This is fairly rough:

This interface is very niche, and not really flexible enough to accommodate other result customization (but I don't think we have any plans here)?

I'm just //summarizing// the content of documents, basically showing the first paragraph of their content, summary, etc. This isn't what Google does: it shows snippets surrounding the actual search terms. However, this is more involved and might be less useful in structured data: for example, I'd imagine that the first line of most phriciton documents, maniphest tasks and Differential revisions really might be the best machine-generatable summary of them. The actual contextual snippeting in Google doesn't often seem hugely useful to me. But this might also not be very useful.

There's not much design, not sure if you had any ideas.

I only implemented this for tasks, revisions and the wiki since those seem most useful.

I'm generally on the fence about this, but it's not a ton of work to swap out for something else later. Maybe we can see how it feels? But happy to toss it or rethink the approach.

Test Plan: {F788026}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8646

Differential Revision: https://secure.phabricator.com/D14095
2015-09-10 19:06:36 -07:00
Joshua Spence
ff6ccd5f68 Remove leftover code for "postponed" lint and unit status
Summary: Ref T9134. It looks like this functionality was removed in D13848.

Test Plan: Submitted a diff successfully.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T9134

Differential Revision: https://secure.phabricator.com/D13869
2015-08-29 22:19:28 +10:00
epriestley
10966519e2 Prevent "commit message magic words" parser from exploding on "reverts aaaa.....aaz"
Summary:
Fixes T9268. Currently, we try to match any string like "a2f313f1" as a commit/revision, so short hashes will get picked up.

However, we don't require a word boundary or terminal after the match, so for input like "aaa...aaaaz" the engine can get stuck trying to split the string into sub-matches.

That is, in the original case, the input "aaaz" had valid matches against `[rA-Z0-9a-f]+` up to "z" of:

  aaa
  aa a
  a aa
  a a a

All of these will fail once it hits "z", but it has to try them all. This complexity is explosive with longer strings.

Instead, require a word boundary or EOL after the match, so this is the only valid match:

  aaa

Then the engine sees the "z", says "nope, no match" and doesn't have to backtrack across all possible combinations.

Test Plan: Added a failing unit test, applied patch, clean test.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9268

Differential Revision: https://secure.phabricator.com/D13997
2015-08-26 09:35:23 -07:00
epriestley
9790f93a83 Show all packages with a claim on a file in the table of contents view
Summary:
Ref T9218. See discussion there for rationale; I think this is the right behavior to pursue.

The screenshot below is pretty ugly. I think it's a lot worse than most real-world cases will be, since you have to sort of opt-in to having crazy levels of overlapping packages, and it's perfectly normal/reasonable for files owned by one package. Owners is powerful enough to let you specify sub-packages with exclusive ownership.

That said, this may be more typical than I hope. I don't think we can reduce the complexity here much for free, but it would might be reasonable to add some view options (e.g.: group by package?, show only packages I own?, show packages as icons with a tooltip?) if it's an issue.

Test Plan: {F734956}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9218

Differential Revision: https://secure.phabricator.com/D13940
2015-08-20 04:46:17 -07:00
Chad Little
e0faa66772 Allow Owners Packages to be archived
Summary: Fixes T8428. Adds status to packages, allows setting and application search. I presume though these need checked elsewhere?

Test Plan: New package, edit package, archive package, run search queries.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T8428

Differential Revision: https://secure.phabricator.com/D13925
2015-08-18 13:36:05 -07:00
epriestley
6010e80e5c Show packages in table of contents views in Diffusion and Differential
Summary:
Fixes T8004.

  - For paths which are part of a package, show the package.
  - Highlight paths which are part of a package you (the viewer) have authority over.

Test Plan:
{F725418}

  - Viewed owned and unowned chagnes in Diffusion and Differential.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8004

Differential Revision: https://secure.phabricator.com/D13923
2015-08-17 10:14:22 -07:00
epriestley
8149399312 Convert commits to use unified table of contents
Summary:
Fixes T2183. We now use the same rendering element in both places.

Intentional changes:

  - Package highlighting is out, coming back to both apps in next diff.
  - removed redundant-feeling "Change" link. The information is now shown with a character ("M", "V", etc.) and the page is a click away under "History". Clicking the path also jumps you to substantially similar content. (We could restore it fairly easily, I just think it's probably the least useful thing in the table right now.)

Test Plan: Viewed a bunch of commits in Diffusion.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2183

Differential Revision: https://secure.phabricator.com/D13910
2015-08-15 12:57:20 -07:00
epriestley
cb912d1735 Convert DifferentialRevision view to new PHUIDiffTableOfContentsListView
Summary:
Ref T2183. Introduces a new View which can (in theory) unify the Revision, Diff and Commit table of contents views.

This has the same behavior as before, but accepts slightly more general primitives and parameters and has somewhat cleaner code.

I've made one intentinoal behavior change: removing the "Open All in Editor" button. I suspect this is essentially unused, and is a pain to keep around. We can look at restoring it if anyone notices.

Test Plan: Looked at a bunch of revisions, no changes from before.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2183

Differential Revision: https://secure.phabricator.com/D13908
2015-08-15 11:43:12 -07:00
epriestley
bb0d13345d Remove unused revisionID and whitespace properties from diff TOC view
Summary: Ref T2183. These properties are not used and not useful.

Test Plan: Grepped for callsites and uses. Loaded revisions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2183

Differential Revision: https://secure.phabricator.com/D13906
2015-08-15 10:54:27 -07:00
Joshua Spence
368f359114 Use PhutilClassMapQuery instead of PhutilSymbolLoader
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.

Test Plan: Poked around a bunch of pages.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13589
2015-08-14 07:49:01 +10:00
epriestley
53ffaaa889 Load buildables in DiffViewController
Summary: Fixes T9128. I missed this when converting other controllers to look in Harbormaster for lint/unit data.

Test Plan: Copy/pasted a diff successfully.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9128

Differential Revision: https://secure.phabricator.com/D13865
2015-08-11 06:39:05 -07:00
Joshua Spence
2cf9ded878 Various linter fixes
Summary: Self explanatory.

Test Plan: `arc lint`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D13863
2015-08-11 22:36:55 +10:00
epriestley
5485fb8aa9 Read modern coverage information for tables of contents
Summary:
Ref T8096. This modernizes the last thing which was reading the old datasource.

Also fix a bug where it didn't work.

Test Plan: {F698405}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13852
2015-08-10 15:24:15 -07:00
epriestley
efa8855e03 Read Differential changeset coverage information from Harbormaster
Summary: Ref T8096. This information has moved into Harbormaster.

Test Plan:
Pushed some test results in; see right margin:

{F698394}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13850
2015-08-10 14:16:36 -07:00
epriestley
60a529b9d1 Remove some obsolete lint and unit support
Summary:
Ref T8096.

Long ago, support for "postponed" lint and unit tests got hacked in. `arc` would publish a bunch of ghost results, and then something else would fill the results in later.

This was always a hack. It is not nearly as powerful or flexible as having a real build system, and is obsolete with Harbormaster, which supports these operations in a more reasonable and straightforward way.

This was used (only? almost only?) at Facebook.

  - Remove `differential.finishpostponedlinters`. This only served to update postponed linters.
  - Remove lint magic in `differential.setdiffproperty`. This magic only made sense in the context of postponed linters.
  - Remove `differential.updateunitresults`. The only made sense for postponed unit tests.

And one minor change: when a diff contains >100 affected files, we hide the content by default, but show content for files with inline comments. Previously, we'd do this for lint inlines, too. I don't tink this is too useful, and it's much simpler to just remove it. We could add it back at some point, but I think large changes often trigger a lot of lint and no one actually cares. The behavior for actual human inlines is retained.

Test Plan: `grep`

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13848
2015-08-10 14:16:25 -07:00
epriestley
2b13da88ce Read synthetic inline comments from Harbormaster
Summary: Ref T8096. These are still reading out of the old diff property, but should read from Harbormaster instead.

Test Plan: {F698346}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13847
2015-08-10 14:16:14 -07:00
epriestley
da1e711abb Hide skipped tests by default in Differential build result summary view
Summary: Ref T8096. Currently, we hide passing tests by default (they aren't interesting) but should also hide skipped tests by default (they aren't interesting either).

Test Plan: {F694485}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13821
2015-08-08 07:43:08 -07:00
epriestley
386d2b62f8 Fix some copy/paste slop with BLOCK actions in Herald transcripts
Summary: Fixes T9060. These actions still work fine, but the transcripts got messed up a bit.

Test Plan: Viewed transcripts with blocking actions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9060

Differential Revision: https://secure.phabricator.com/D13782
2015-08-04 07:32:19 -07:00
epriestley
5f76c71d78 Share target filtering code in HeraldAction
Ref T8726. This shares some target filtering code with the base class.
2015-08-03 14:34:37 -07:00
epriestley
a3e2f655eb Modularize the Diffusion/Differential "Block" Herald actions
Also removes HeraldCustomAction. This completes action modularization.

Ref T8726.
2015-08-03 14:33:28 -07:00
epriestley
6f6d88794b Modularize the Diffusion "Add Auditors" Herald action
Ref T8726.
2015-08-03 14:33:27 -07:00
epriestley
776caa507b Modularize the Harbormaster "Run build plan" Herald action
Ref T8726. Modularizes "Run build plan" in Differential and Diffusion.
2015-08-03 14:33:26 -07:00
epriestley
fdd379a026 Modularize the Legalpad "Require Signature" Herald Action
Ref T8726. Modularizes "Require Signatures" for Legalpad.
2015-08-03 14:33:26 -07:00
epriestley
a335004a91 Modularize Differential Reviewer actions in Herald
Ref T8726. Modularizes the "Add Reviewers" and "Add Blocking Reviewers" Herald actions.
2015-08-03 14:33:25 -07:00
epriestley
51fead17cf Modularize "Send an Email" Herald actions
Summary: Ref T8726. No surprises.

Test Plan:
Created rules using both action variants, applied upgrade, saw rules still work correctly.

{F658842}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13701
2015-08-03 14:33:23 -07:00
epriestley
8ae08a3de7 Make "Add Subscribers" and "Remove Subscribers" Herald actions modular
Summary: Ref T8726. Converts these actions to be modular. No real surprises in this change.

Test Plan:
{F658709}

  - Wrote some rules.
  - Migrated them forward.
  - Used a bunch of these rules.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13699
2015-08-03 14:33:22 -07:00
epriestley
8d9bd791f7 Modularize Herald "flag" action, plus update transcripts
Summary:
Ref T8726. This modularizes "Mark with flag", plus rebuilds transcripts in a more modern/flexible way. The big transcript stuff is:

  - Transcripts are now translatable.
  - Transcripts can now show multiple outputs from a single action. For example, an action like "add A, B, C to subscribers" can now say "added A; B is invalid; C was already subscribed".

Test Plan: {F637784}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, eadler, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13649
2015-08-03 14:33:21 -07:00
epriestley
56dd5211f0 Make Herald action modularization more aggressive
Summary:
Ref T8726. Herald actions are technically sort-of modular already, but make them more aggressively modular similar to `HeraldField`.

I plan to obsolete and replace `HeraldCustomAction`.

Test Plan: Saw actions in nice groups; created and ran a "Do Nothing" action. Transcripts are a bit rough for now.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13646
2015-08-03 14:33:20 -07:00
epriestley
f1222f956a Correctly clear draft markers when deleting an inline comment
Summary:
Fixes T8917. Prior to T2618, deleting inlines prompted users, then really deleted the rows.

After T2618, we delete immediately and offer "Undo". However, some interactions with drafts were missed, and we were only clearing the "this revision has a draft" flag on one of the delete pathways (when you delete all the comment text, then save the comment).

Make both the "Delete" action and the "Delete All Comment Text + Save" workflows do the same thing: mark the row as deleted, and clear any relevant drafts.

Test Plan:
  - Made an inline comment on a clean revision with no "draft comments" marker in the list view.
  - Used "delete" to delete it.
  - After applying the patch, verified that no "draft commetns" marker appears in the list view.
  - Used Delete and Edit + Remove Text + Save to delete comments in Differential and Diffusion.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T8917

Differential Revision: https://secure.phabricator.com/D13665
2015-07-21 11:36:46 -07:00
epriestley
0306eb70ed Group and order Herald fields
Summary:
Ref T8726. Some adapters now have a large number of fields, and we lost the sort-of-human-readable implicit ordering when fields were modularized.

Instead, group and sort fields.

Test Plan: {F603066}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13619
2015-07-16 14:13:13 -07:00
epriestley
715233fb61 Fully modularize Herald field values
Summary: Ref T8726. This gets rid of all the `VALUE_*` constants and lets Fields provide arbitrary typeaheads without upstream/JS changes.

Test Plan: Used all tokenizers. Used "Another Herald Rule". Grepped for all removed constants.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13616
2015-07-16 14:12:44 -07:00
epriestley
b2b739c709 Reduce code duplication in Herald fields
Summary:
Ref T8726. I want to modularize values and reduce how hard-coded / copypasta'd they are.

  - Rename `get...StandardCondition()` to `get...StandardType()`, since we can drive both conditions and values from it.
  - Rename `STANDARD_LIST` to `STANDARD_PHID_LIST` for consistency: all "lists" are lists of PHIDs.
  - For all standard types which don't require typehaeads, lift their logic into the base class.
  - I'll lift typeaheads soon, but need to generalize them first.

Test Plan: Edited various Herald rules, saw value UI generate correctly.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13612
2015-07-16 14:11:44 -07:00
epriestley
00b98eca54 Modularize all DifferentialRevision fields
Summary: Ref T8726. Make all the DifferentialRevision stuff modular.

Test Plan:
  - Created a rule with all fields.
  - Ran upgrade.
  - Saw all fields preserved with new modular versions.
  - Used test console to run rule with all fields, verified field values as broadly sensible.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13598
2015-07-08 12:25:48 -07:00
epriestley
c013108675 Modernize all Herald DifferentialDiff fields
Summary: Ref T8726. This deals with all the Differential diff fields, same deal as previous changes.

Test Plan:
  - Wrote a rule with every field.
  - Migrated it.
  - Saw the same rule working.
  - Rigged the hell out of transcripts (diffs normally do not generate transcripts, because the only action is "block" and they don't exist yet when Herald runs).
  - Verified that all fields looked sensible in the transcript.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13590
2015-07-08 12:25:38 -07:00
Joshua Spence
0036670329 Remove remaining arcanist project code
Summary: Fixes T7604. This is the big scary change which drops the "arcanist project" fields from the database permanently.

Test Plan:
`grep`ped for the following to ensure that I had found all remaining references:

  - `/arcanistProject/i`
  - `/arcanist_project/i`
  - `/projectName/i`
  - `/project_name/i`
  - `/project_id/i`

WARNING: Wait at least one month before landing this.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7604

Differential Revision: https://secure.phabricator.com/D12899
2015-07-08 19:37:28 +10:00
Joshua Spence
f2435fd1d0 Return $this from setter methods
Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.

Test Plan: Eyeball it.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13422
2015-07-06 22:53:43 +10:00
Chad Little
66afb92fb3 [Redesign] Add color to diff Table of Contents
Summary: Ref T8099. I think these colors make sense, but if any seem wrong, lmk. Colors the type of file change.

Test Plan: Test a few diffs locally, read carefully, ask epriestley.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T8099

Differential Revision: https://secure.phabricator.com/D13502
2015-07-01 11:33:31 -07:00
epriestley
14a395ed8e Merge branch 'master' into redesign-2015 2015-06-25 10:06:50 -07:00
epriestley
db1bc7fd7f Carefully avoid the harbormaster/differential race
Summary: Ref T8650. This should stop the problem, but isn't a root cause fix. See discussion on the task.

Test Plan: Made some local diffs, but this is a bit hard to reproduce reliably.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8650

Differential Revision: https://secure.phabricator.com/D13441
2015-06-25 10:05:37 -07:00
epriestley
1a5f986d73 Merge branch 'master' into redesign-2015 2015-06-23 17:12:41 -07:00
epriestley
520e70a3cf Fix a bad method call which slipped by
Fixes T8650.
2015-06-23 17:11:59 -07:00
epriestley
075f041bec Merge branch 'master' into redesign-2015 2015-06-23 13:36:47 -07:00
epriestley
716bd4e4b4 Improve lint/unit limit, sort, view all, collapse behaviors
Summary:
Ref T8096. Various tweaks here:

  - Sort result lists by importance (even lint -- "errors first" seems better than "alphabetical by file", I think?).
  - Do sane stuff with display limits.
  - Add a "view all" view.
  - Don't show a huge table of passing tests in Differential.
  - Link to full results.

Test Plan: See screenshots.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13407
2015-06-23 13:34:25 -07:00
epriestley
b074cdeb4c Reduce Lint/Unit rendering duplication
Summary: Ref T8096. No functional changes, just a bit less code.

Test Plan: Viewed some revisions, saw the same stuff as before.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13404
2015-06-23 13:33:09 -07:00
epriestley
524aa39aa2 Merge branch 'master' into redesign-2015 2015-06-23 10:26:47 -07:00
epriestley
de30e15b7e Make Differential load lint/unit data from Harbormaster
Summary: Fixes T8095. Still needs UI/UX work (see T8096) but this has all the core features now.

Test Plan: Saw Harbormaster lint/unit data as though it was Differential lint-unit data.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13401
2015-06-23 10:23:52 -07:00
epriestley
c31e25d5ce Smooth out some UI/UX issues in Harbormaster
Summary:
Ref T8096. Fixes a few bugs and glitches.

  - Set build completion time when handling a message.
  - Format duration information in a more human-readable way.
  - Use a table for build variables.
  - Fix up container PHIDs on diffs (a touch hacky, should be OK for now though).

Test Plan: Browsed around the UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13382
2015-06-23 10:23:35 -07:00
epriestley
41b3f9236a Allow lint and unit results to be reported via harbormaster.sendmessage
Summary:
Ref T8095. When build results are reported for a target, allow them to include unit and lint results.

There is no real way to see this stuff in the UI yet, either in Harbormaster or Differential.

Test Plan: Manually called this method with some results, saw Harbormaster update appropriately.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13380
2015-06-23 10:23:33 -07:00
epriestley
831c18e6be Remove weird "Differential Results Table" view
Summary: Ref T8095. This weird grey table has no remaining callsites and can be removed.

Test Plan: Grepped for symbols.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13379
2015-06-23 10:23:30 -07:00
epriestley
b72deb332c Render unit results as Harbormaster unit messages
Summary:
Ref T8095. Same as D13377, but for unit results.

This is a bit rough and there's some duplication between this and unit results. I'll likely merge them later, but I think some of it is superficial since these iterations are still a little crude.

Test Plan: {F523499}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13378
2015-06-23 10:23:29 -07:00
epriestley
e618b672d2 Render lint results as Harbormaster lint messages
Summary:
Ref T8095. Render lint results in a future-ready way.

This makes the renderer accept `HarbormasterBuildLintMessage` objects. If we have legacy data instead, it converts it into `HarbormasterBuildLintMessage` objects.

Design is a bit rough but will be cleaned up later after T7739.

This moves away from "postponed linters", which are obsolete after Harbormaster (and were only ever used by Facebook).

Test Plan: {F523429}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13377
2015-06-23 10:23:28 -07:00
Chad Little
d27b3146d2 [Redesign] Normalize header policy and object tags
Summary: Ref T8099, This adds a consistent background color to object and policy tags, and highlights them when they deviate from the normal. Still likely worth revamping 'closed' and 'review' state colors.

Test Plan: Review lots of diffs and tasks.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T8099

Differential Revision: https://secure.phabricator.com/D13399
2015-06-23 12:03:46 +01:00
epriestley
b55f9b6120 Merge branch 'master' into redesign-2015 2015-06-22 12:26:41 -07:00
epriestley
d1983560a6 Show when objects have a non-default policy
Summary:
Fixes T6787. I'm kind of cheating a little bit here by not unifying default selection with `initializeNew(...)` methods, but I figure we can let this settle for a bit and then go do that later. It's pretty minor.

Since we're not doing templates I kind of want to swap the `'template'` key to `'type'` so maybe I'll do that too at some point.

@chad, freel free to change these, I was just trying to make them pretty obvious. I //do// think it's good for them to stand out, but my approach is probably a bit inconsistent/heavy-handed in the new design.

Test Plan:
{F525024}

{F525025}

{F525026}

{F525027}

Reviewers: btrahan, chad

Reviewed By: btrahan

Subscribers: johnny-bit, joshuaspence, chad, epriestley

Maniphest Tasks: T6787

Differential Revision: https://secure.phabricator.com/D13387
2015-06-22 11:46:59 -07:00
epriestley
76194a0dc1 Add "Autoplans" to Harbormaster
Summary:
Ref T8095. Two general problems:

  - I want Harbormaster to own all lint and unit test results.
  - I don't want users to have to configure anything for `arc` to keep working automatically.

These are in conflict because generic lint/unit test ownership in Harbormaster requires that build targets exist which we can attach build results to. However, we can't currently create build targets on demand: Harbormaster assumes it is responsible for creating targets, then running code or making third-party service calls to actually run the builds.

I considered two broad approaches to let `arc` push results into Harbormaster without requiring administrators to configure some kind of "arc results" build plan:

  # Add magic target PHIDs like `PHID-MAGIC-this-is-really-arc-unit`.
  # Add new code to build real targets with real PHIDs.

(1) is probably a bit less work to get off the ground, but I think it's worse overall and very likely to create more problems in the long run. I particularly worry that it will lead to a small amount of special casing in a very large number of places, which seems more fragile.

(2) is more work upfront but I think does a better job of putting all the special casing in one place that we can, e.g., more reasonably unit test, and letting the rest of the code rarely/never care about this case since it's just dealing with normal plans/steps/targets as far as it can tell.

This diff introduces "autoplans", which are source templates for plans/steps. This let us "push" these targets into Harbormaster. Hypthetically, any process "like" arc can use autoplans to upload test/lint/etc results. In practice, probably only `arc` will ever use this, but I think it's still quite a bit cleaner than the alternative despite all the generality.

Workflow is basically:

  - `arc` creates a diff.
  - `arc` calls `harbormaster.queryautotargets`, passing the diff PHID and saying "I have some lint and unit results I want to stick on this thing".
  - Harbormaster builds the plan, steps, and targets (if any of them don't already exist), and hands back the target PHIDs so `arc` has a completely standard-looking place to put results.
  - `arc` uploads the test results to the right targets, as though Harbormaster had asked it to run unit/lint in the first place.

(This doesn't actually do any of that yet, just sets things up.)

I'll maybe doc turn that ^^^^^^ into a doc for posterity since I think it's hard to guess what an "autotarget" is, but I'm going to grab some lunch first.

Test Plan:
  - Added unit tests to make sure we can build these things properly.
  - Used `harbormaster.queryautotargets` to build autotargets for a bunch of diffs.
  - Verified targets come up in "waiting for message" state.
  - Verified plans and steps are not editable.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: hach-que, epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13345
2015-06-21 09:04:21 -07:00
Chad Little
eba1153e7e [Redesign] Provide more color to object states in headers
Summary: Ref T8099, Now that we have cleaner headers, we can add more pop to important items, like the state of an Object. This makes it easier to just note the color and generally understand the state of the object (closed, returned, accepted, open). @epriestley, I think you previously thought this was a bug, but if it still feels bad, let me know.

Test Plan:
Review Differential, Maniphest, Polls for various object states.

{F520973}

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T8099

Differential Revision: https://secure.phabricator.com/D13369
2015-06-21 13:38:07 +01:00
Chad Little
801607381d [Redesign] PhabricatorApplicationSearchResultView
Summary: Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?

Test Plan: Review and do a search in each application changed. Grep for all call sites.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T8099

Differential Revision: https://secure.phabricator.com/D13332
2015-06-19 11:46:20 +01:00
epriestley
29ac80be5c Merge branch 'master' into redesign-2015 2015-06-17 11:27:12 -07:00
epriestley
b3ae48d8ca Improve Differential query plans
Summary:
Ref T8575. We run a big "(A) UNION (B)" query on the home page and on the main Differential page.

"A" can always be improved by using `%Ls`, so it can use the second half of the `(authorPHID, status)` key.

"B" can sometimes be improved if the fraction of open revisions is smaller than the fraction of revisions you are reviewing. This is true for me on secure.phabricator.com (I'm a reviewer, either directly or via 'Blessed Reviewers', on about 80% of revisions, but <5% are open). In these cases, a `(status, phid)` key is more efficient.

Test Plan: Tweaked queries and added keys on this server, saw less derpy query plans and performance.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8575

Differential Revision: https://secure.phabricator.com/D13325
2015-06-17 11:25:01 -07:00
epriestley
b97d7dddc8 Merge branch 'master' into redesign-2015 2015-06-16 13:11:59 -07:00
epriestley
2a5cf5e1b7 Separate "Revision" and "Diff" fields in Differential
Summary:
Fixes T5138. Some of the "revision" properties are really "diff" properties, but we only show the properties for the most recent / current diff.

  - Immediately, this makes it hard or impossible to review, e.g., lint/unit results for older diffs.
  - Longer-term, these limits will become more problematic with more data on diffs after Harbormaster.

Instead, separate "revision" from "diff" properties.

(In the long term, it might make sense to show more diffs in this panel -- e.g., tabs for the 8 most recent updates or something -- but I went with the simplest approach for now since I don't have a clean way to deal with 100-update revisions offhand.)

Test Plan: {F500480}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: cburroughs, epriestley

Maniphest Tasks: T5138

Differential Revision: https://secure.phabricator.com/D13282
2015-06-16 08:53:40 -07:00
epriestley
53ef057b1b Merge branch 'master' into redesign-2015 2015-06-15 08:06:23 -07:00
Joshua Spence
b6d745b666 Extend from Phobject
Summary: All classes should extend from some other class. See D13275 for some explanation.

Test Plan: `arc unit`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13283
2015-06-15 18:02:27 +10:00
Joshua Spence
175799fab9 Remove break from hunk migration
Summary: This `break` statement causes `./bin/hunks migrate` to only migrate one-hunk-at-a-time, which is unnecessary and slightly misleading. Instead, allow the script to migrate //all// legacy hunks to modern storage. In particular, this means that we can recommend that installs run this command sometime before D13222 is landed.

Test Plan: It's a pain to setup the data necessary to test this, but this is identical to the change that I made on our production install when I migrated our hunk storage.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13288
2015-06-15 08:32:08 +10:00
epriestley
52f8756c3c Add a "template" parameter to application default policies
Summary:
Ref T5681. Ref T6860. This doesn't do anything interesting on its own, just makes the next diff smaller.

In the next diff, policies become aware of the types of objects they're acting on. We need to specify which object type all the "Default View/Edit" settings are for so they get the right rules.

For example, a rule like "Allow task author" is OK for "View Policy" on a task, and also OK for "Default View Policy" on ManiphestApplication. But it's not OK for "Can Create Tasks" on ManiphestApplication.

So annotate all the "template"/"default" policies with their types. The next diff will use these to let you select appropriate rules for the given object type.

Test Plan:
  - Used `grep` to find these.
  - This change has no effect.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5681, T6860

Differential Revision: https://secure.phabricator.com/D13251
2015-06-11 13:25:30 -07:00
epriestley
57b898af9a Merge branch 'master' into redesign-2015 2015-06-10 07:44:58 -07:00
Joshua Spence
f47e69c015 Mark some strings for translation
Summary: Add some more `pht`izations.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: hach-que, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D13200
2015-06-09 23:06:52 +10:00
epriestley
929f5f22ac Fix an issue with adding or updating images in unified diff view
Summary: Fixes T8069.

Test Plan: Viewed a new image in unified diff view.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8069

Differential Revision: https://secure.phabricator.com/D13217
2015-06-08 15:40:10 -07:00
epriestley
9e88ede69d Use standard subscribers effects in Herald Adapter for revisions
Summary:
Ref T8455. Use standard effects for revisions, instead of a custom effect.

This fixes the major issue (conduit error) in T8455 because the standard effect now performs PHID type filtering.

This retains other behaviors (in particular: not re-CC'ing explicitly removed CCs).

Test Plan:
  - With a Herald rule that adds a mailing list as a CC, created a revision before the change and hit the error in T8455. After the change, saw correct behavior.
  - Wrote a normal Herald rule to add CCs and created a revision, saw it fire properly.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8455

Differential Revision: https://secure.phabricator.com/D13183
2015-06-08 10:32:08 -07:00
epriestley
623aaf488d Consolidate "Subscribers" Herald field value logic
Summary:
Ref T8455. The Herald code in general isn't nearly as modular as it should be, and the subscriber code particularly has some legacy cruft. This is making it fragile and causing the issue described in T8455.

Currently, each Herald adapter has essentially identical code which it uses to determine which users are subscribed to an object. Instead, share code between object types.

I removed "explicitCCs":

  - The value was always identical to doing the query in the common/standard way.
  - They were only used to print a diagnostic message on transcripts, which I think is no longer relevant.
    - I believe it predates transactions, so when it was added you couldn't figure out the old object state by looking at the transaction history. Now, CC changes are recorded there, so there's no need to restate the CC state on the transcript.
    - Even if we do want to restore this (or something similar), we can do it directly from Herald now.

Test Plan:
  - Created rules that use the "CCs" field in Herald, Pholio, Maniphest and Differential.
  - Updated objects in each application.
  - Observed valid field reads in the tranascript.
  - Grepped for `FIELD_CC`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8455

Differential Revision: https://secure.phabricator.com/D13177
2015-06-08 10:18:01 -07:00
epriestley
2e0f189950 Fix an issue with extended policy checks and @mentions
Summary: Ref T8463.

Test Plan:
  - Created a new revision via web UI with a username `@mention` in the summary and no repository.
  - Prior to patch, hit a "not attached" error.
  - After patch, no error.
  - Created a new web UI revision, as above, but with a repository; saw repository work fine.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8463

Differential Revision: https://secure.phabricator.com/D13205
2015-06-08 10:07:05 -07:00
Joshua Spence
5914bbd806 Remove *TransactionType classes
Summary: Remove the `*TransactionType` classes and define the constants in the corresponding `*Transaction` class instead.

Test Plan: `grep`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D13188
2015-06-08 11:26:43 +10:00
epriestley
4a45620b04 Reload revisions before publishing mail about them
Fixes this, which only triggers on some kinds of mail:

```
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] [2015-06-04 02:56:38] EXCEPTION: (PhutilProxyException) Error while executing Task ID 902251. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on DifferentialRevision (via getActiveDiff()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=6dede2e2c513), phutil(head=master, ref.master=afc05a9a7f00)
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/differential/storage/DifferentialRevision.php:158]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #1 <#2> DifferentialRevision::getActiveDiff() called at [<phabricator>/src/applications/differential/customfield/DifferentialBranchField.php:72]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #2 <#2> DifferentialBranchField::updateTransactionMailBody(PhabricatorMetaMTAMailBody, DifferentialTransactionEditor, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2481]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #3 <#2> PhabricatorApplicationTransactionEditor::addCustomFieldsToMailBody(PhabricatorMetaMTAMailBody, DifferentialRevision, array) called at [<phabricator>/src/applications/differential/editor/DifferentialTransactionEditor.php:1208]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #4 <#2> DifferentialTransactionEditor::buildMailBody(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2178]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #5 <#2> PhabricatorApplicationTransactionEditor::sendMailToTarget(DifferentialRevision, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2152]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #6 <#2> PhabricatorApplicationTransactionEditor::sendMail(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:998]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #7 <#2> PhabricatorApplicationTransactionEditor::publishTransactions(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #8 <#2> PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #9 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #10 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:20]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #11 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]   #12 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```

Auditors: btrahan
2015-06-03 20:01:23 -07:00
epriestley
a9ceebbdb1 Move all ApplicationTransaction publishing to daemons
Summary:
Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.

There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.

Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:

  - This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them.
  - This is more correct, since we want to freeze the lists at this moment in time.

Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook.

---

This is a bit fragile and I'm not //thrilled// about it.

It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.

Test Plan:
Directly updated editors:

- Created a new Phriction page, saw "Document Content".
- Edited a Phriction page, saw "Document Diff".
- Edited a revision, got normal looking mail.
- Faked in `changedPriorToCommitURI` and verified it survived the state boundary.
- Sent Audit mail.
- Sent invoice mail.

Indirect editors - for these, I just made a change and made sure the mail generated:

- Updated a paste.
- Updated an event.
- Updated a thread.
- Updated a task.
- Updated a mock.
- Updated a question.
- Updated a project.
- Updated a file.
- Updated an initiative.
- Updated a Legalpad document.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, fabe

Maniphest Tasks: T6367

Differential Revision: https://secure.phabricator.com/D13115
2015-06-03 18:59:29 -07:00
epriestley
d6247ffca5 Add support for "Extended Policies"
Summary:
Ref T7703. See that task and inline for a bunch of discussion.

Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later.

We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times.

Test Plan:
  - Added and executed unit tests.
  - Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions.
  - Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7703

Differential Revision: https://secure.phabricator.com/D13104
2015-06-03 18:59:27 -07:00
epriestley
ba6cb62b49 Remove mailing lists application
Summary: Ref T8387. This is now completely obsoleted by mailing list users.

Test Plan: Grepped for `mailinglist` and related symbols.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: eadler, epriestley

Maniphest Tasks: T8387

Differential Revision: https://secure.phabricator.com/D13129
2015-06-03 18:42:36 -07:00
Joshua Spence
bf81fda036 Linter fixes
Summary: Apply various minor linter fixes.

Test Plan: `arc lint`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D13109
2015-06-02 22:14:01 +10:00
Joshua Spence
9c5d9e3f47 Remove "arcanist project" fields
Summary: Ref T7604. Remove these "arcanist project" fields from the `LiskDAO` classes, but leave the data intact (there are no more read/writes to this data).

Test Plan:
- Grepped for calls to these methods.
- Ran `./bin/storage adjust` to check for schema issues.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7604

Differential Revision: https://secure.phabricator.com/D13012
2015-06-01 09:54:48 +10:00
Chad Little
2ec7cdcfba [Redesign] Use normal sized buttons for Changesets
Summary: Ref T8099, No specific reason for these to be small buttons.

Test Plan: Test desktop and mobile layouts.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T8099

Differential Revision: https://secure.phabricator.com/D13081
2015-05-31 07:53:57 -07:00
Chad Little
b0056dd16c [Redesign] Try fa-code icon for open Diffs
Summary: Ref T8099, changes the icon from open to 'code'. Feels slightly better.

Test Plan: btrahan, epriestley

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T8099

Differential Revision: https://secure.phabricator.com/D13055
2015-05-28 13:17:57 -07:00
epriestley
3699253e49 Merge branch 'master' into redesign-2015 2015-05-28 12:00:06 -07:00
epriestley
5aa4044a9d Fix synthetic (lint) inline comments for comment hiding
Summary: These could cause F442758.

Test Plan: {F442779}

Reviewers: btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D13042
2015-05-27 15:47:18 -07:00
epriestley
e9f4a84a89 Allow inline comments to be individually hidden
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.

This is sticky per-user.

My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).

Specifically, this adds a new action here:

{F435621}

Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:

{F435626}

You can click the icon to reveal all hidden comments below the line.

Test Plan:
  - Hid comments.
  - Showed comments.
  - Created, edited, deleted and submitted comments.
  - Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).

Reviewers: btrahan, chad

Reviewed By: btrahan

Subscribers: jparise, yelirekim, epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D13009
2015-05-27 10:28:38 -07:00
Aviv Eyal
08a9e0f22a Hide Land to GitHub
Summary:
We don't want to support this right now, so disable it, similar to "Land To Hosted" feature.
Keep to the code as an example for advanced installs.

Ref T182, T8313

Test Plan: load a diff.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T182, T8313

Differential Revision: https://secure.phabricator.com/D13022
2015-05-26 11:28:07 -07:00
Joshua Spence
1b12249b2c Fix some format strings
Summary: These format strings use `%d` instead of `%s`.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12996
2015-05-25 21:29:30 +10:00
epriestley
bfca11dbba Merge branch 'master' into redesign-2015 2015-05-22 12:57:32 -07:00
Joshua Spence
36e2d02d6e phtize all the things
Summary: `pht`ize a whole bunch of strings in rP.

Test Plan: Intense eyeballing.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: hach-que, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12797
2015-05-22 21:16:39 +10:00
Aviv Eyal
f21972a01f Only link symbols if there might be any
Summary:
fixes T8260. Only turn on symbol links if:
- The repository has any configuration about symbols, or
- There actually are symbols in the repository.

Test Plan: Look at revisions and files in various states of configurations and having symbols.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: joshuaspence, Korvin, epriestley

Maniphest Tasks: T8260

Differential Revision: https://secure.phabricator.com/D12946
2015-05-21 08:25:35 -07:00
Chad Little
a4784e03ff [Redesign] Add Table, Collapse support to ObjectBox
Summary: Converts most all tables to be directly set via `setTable` to an ObjectBox. I think this path is more flexible design wise, as we can change the box based on children, and not just CSS. We also already do this with PropertyList, Forms, ObjectList, and Header. `setCollapsed` is added to ObjectBox to all children objects to bleed to the edges (like diffs).

Test Plan: I did a grep of `appendChild($table)` as well as searches for `PHUIObjectBoxView`, also with manual opening of hundreds of files. I'm sure I missed 5-8 places. If you just appendChild($table) nothing breaks, it just looks a little funny.

Reviewers: epriestley, btrahan

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12955
2015-05-20 12:48:43 -07:00
Chad Little
667159eb07 [Redesign] Add Table, Collapse support to ObjectBox 2015-05-19 23:14:22 -07:00
Joshua Spence
69940f2b9e Replace ArcanistPhutilTestCase refs with PhutilTestCase
Summary: Ref T7977. Remove the `PhabricatorTestCase::getLink` method. Depends on D12665.

Test Plan: `arc unit`.

Reviewers: avivey, #blessed_reviewers, epriestley

Reviewed By: avivey, #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7977

Differential Revision: https://secure.phabricator.com/D12667
2015-05-20 09:40:39 +10:00
Bob Trahan
2154f17b3d Transactions - fix bug adding people to projects
Summary: Fixes T8264. Broken in D12929. Sweep all the applyBuiltin implementations and always break; rather than return

Test Plan: added myself to a project successfully (showed up as a member)

Reviewers: epriestley, chad

Subscribers: Korvin, epriestley

Maniphest Tasks: T6403, T8264

Differential Revision: https://secure.phabricator.com/D12940
2015-05-19 15:22:41 -07:00
Bob Trahan
81a475d5a6 Transactions - make implementing TYPE_XXXX_POLICY transactions optional
Summary: Ref T6403. This was actually simple stuff.

Test Plan: changed the edit policy of a paste. changed the edit and join policy of a phame blog.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6403

Differential Revision: https://secure.phabricator.com/D12933
2015-05-19 12:58:18 -07:00
Bob Trahan
01a8ba5a97 Transactions - make TYPE_COMMENT implementation optional
Summary: Ref T6403. Conpherence keeps track of comments for message counts so we needed some special attention there. Otherwise, straight-forward.

Test Plan: left a comment on a diff with inline comments. sent messages in conpherence successfully. verified unread count incremented correctly for sent messages for users.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6403

Differential Revision: https://secure.phabricator.com/D12932
2015-05-19 12:33:55 -07:00
Bob Trahan
18e0ee0791 Transactions - move TYPE_SUBSCRIBERS to require optional implementation
Summary: Ref T6403. This one was pretty easy since no one does anything custom with subscribers.

Test Plan: subscribed / unscribed to a random commit ("audit"). joined / left, watched / unwatched a project

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6403

Differential Revision: https://secure.phabricator.com/D12930
2015-05-19 11:48:02 -07:00
Bob Trahan
16c8d44c37 Transactions - make customization of TYPE_EDGE optional
Summary: Ref T6403. This does TYPE_EDGE since I just had to deal with T8252. Look like this fixes a few editors (maybe) that would have had fatals with mentions like slowvote and ponder.

Test Plan: made a phame post mentioning a task and it worked! joined / left a project, watched / unwatched a project and that worked! blind faith for other sites.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6403

Differential Revision: https://secure.phabricator.com/D12929
2015-05-19 11:26:53 -07:00
Joshua Spence
021223907d Remove arcanist projects from differential
Summary: Ref T7604. Remove arcanist projects from differential. Depends on D12687 and D12893.

Test Plan: Submitted a diff. Patched the diff with `arc patch`.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7604

Differential Revision: https://secure.phabricator.com/D12894
2015-05-19 00:36:52 +10:00
Aviv Eyal
898ce6bace Search Symbols by Repository, not Project
Summary:
Fixes T7977.
- Move Indexed Languages and See Symbols From config to Repository
- Make symbol search skip projects

This also makes the default languages to Everything instead of Nothing.

Test Plan:
- Browse files, click symbols.
- Use quick search to find symbols
- Browse revision, click symbols

Reviewers: joshuaspence, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7977

Differential Revision: https://secure.phabricator.com/D12687
2015-05-18 06:41:19 -07:00
Bob Trahan
3ef0721ada Reduce PhabricatorUser::getOmnipotentUser calls by adding a getViewer method to PhbaricatorDestructionEngine
Summary:
Fixes T6956. Before this change, we called PhabricatorUser::getOmnipotentUser in the various delete methods to query the data. Now, we use $engine->getViewer(), since its always a good thing to have less calls to PhabricatorUser::getOmnipotentUser thrown around the codebase.

I used the "codemod" tool to audit the existing calls to PhabricatorDestructorEngine (all of them) so ostensibly this gets all the spots. If I missed something though, its still going to work, so this change is very low risk.

Test Plan: ./bin/remove destroy P1; visit P1 and get a 404

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6956

Differential Revision: https://secure.phabricator.com/D12866
2015-05-15 14:07:17 -07:00
Chad Little
46f6c2680e [Redesign] New PHUIObjectItemListView
Summary:
New, cleaner, ObjectItemLists. Lots of minor style tweaks, basic overview:

 - Remove FootIcons
 - Remove Stackable
 - Remove Plain List
 - Add StatusIcon
 - Add setting ObjectList to an ObjectBox
 - Minor retouches to Headers

Mostly, this should give us an idea of life with the new Object Lists. I'll take another application by application pass down the road. This mostly looks at implementation in Maniphest, Differential, Audit, Workboards. Checked a few other areas and dialogs while testing, and everything looks square.

Test Plan: Maniphest, Differential, Homepage, Audit, People, and other applications. Drag reorder, etc.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12865
2015-05-15 13:28:59 -07:00
epriestley
04186e02cf Separate handle "status" and "availability"
Summary:
Ref T7707. Handles currently have a "status" field and a "disabled" field.

The "status" field has these possible values: "open", "closed", "1", "2". durp durp durp

Instead, do:

  - status = <open, closed>
  - availability = <full, partial, none, disabled>

I think these make more sense? And are a bit more general? And use the same kind of constants for all values!

Test Plan: Looked at all affected handles in all states (probably).

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7707

Differential Revision: https://secure.phabricator.com/D12832
2015-05-14 11:14:44 -07:00
Joshua Spence
61b178f44e Use PhutilInvalidStateException
Summary: Use `PhutilInvalidStateException`. Depends on D12803.

Test Plan: Unit tests pass.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12829
2015-05-14 07:53:52 +10:00
Joshua Spence
acb45968d8 Use __CLASS__ instead of hard-coding class names
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.

Test Plan: Eyeball it.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: hach-que, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12806
2015-05-14 07:21:13 +10:00
epriestley
a238f6a759 Implement rough content-aware inline adjustment rules for ghosts
Summary:
Ref T7447. Fixes T7600. This likely needs significant adjustment, but implements content-aware comment porting for line changes.

Specifically, this moves lines around to adjust their position considering added and removed lines between the diffs and across rebases.

It does not try to do any actual content (line against line) matching.

Test Plan:
  - Unit tests.
  - Poking around in the web UI seems to generate mostly reasonable-ish results?
  - This may be a huge step backward in some cases that I just haven't hit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T7600, T7447

Differential Revision: https://secure.phabricator.com/D12741
2015-05-07 14:09:41 -07:00
epriestley
88b1ad7e92 Allow Phabricator to parse bare revision IDs from "Differential Revision:" fields
Summary: Fixes T8087. Depends on D12748.

Test Plan: See D12748.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: cburroughs, epriestley

Maniphest Tasks: T8087

Differential Revision: https://secure.phabricator.com/D12749
2015-05-07 11:10:23 -07:00
epriestley
abe28e628a Improve performance of generating synthetic changesets
Summary: Ref T7776. This could get better, but I think I got most of the big stuff. It's ~4x faster now.

Test Plan:
Before:

{F393338}

After:

{F393339}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7776

Differential Revision: https://secure.phabricator.com/D12730
2015-05-06 16:43:32 -07:00
epriestley
336ccfeea1 Revert a fourth json_decode() which decodes possible scalars
See D12714, D12680.

Auditors: joshuaspence
2015-05-05 13:13:08 -07:00
Joshua Spence
70c8649142 Use phutil_json_decode instead of json_decode
Summary: Generally, `phutil_json_decode` should be preferred over `json_decode`.

Test Plan: Eyellballed.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12680
2015-05-05 20:48:55 +10:00
epriestley
29feca3c1e Add a preference to disable ghost comments
Summary: Ref T7447. At least some users dislike this feature so strongly that they'd prefer not to have it at all.

Test Plan: Viewed ghosts; toggled preference, no more ghosts.

Reviewers: chad

Reviewed By: chad

Subscribers: yelirekim, epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D12704
2015-05-04 12:02:42 -07:00
epriestley
460d68ba27 Provide link to inlines in-context in $ghost['href']
Summary: See D12702.

Test Plan: Made something a link and clicked it, seemed to work OK.

Reviewers: chad

Reviewed By: chad

Subscribers: yelirekim, epriestley

Differential Revision: https://secure.phabricator.com/D12703
2015-05-04 11:52:21 -07:00
lkassianik
3b226e8fb3 Calendar event monograms, part 4. Typing E{id} makes the object available in global search.
Summary: Closes T7928, E{id} is available via global search.

Test Plan: Create calendar event, search for its monogram in global search, event should be accessible.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7928

Differential Revision: https://secure.phabricator.com/D12581
2015-04-27 15:31:18 -07:00
Chad Little
12dc865a00 Expand typeahead document copy
Summary: Seems reasonable? At least, it always matches however a user might think about documents (app or document). Unclear if "Diffusion" for example, are actually needed.

Test Plan: tested searching for "phriction", "wiki", "document", etc.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12577
2015-04-27 10:06:38 -07:00
epriestley
bcb957bc8a Use function datasources for a couple more Differential fields
Summary: "Authors" and "Subscribers" convert easily without any extra work.

Test Plan: Used both fields; used functions and normal tokens.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12556
2015-04-27 03:52:24 -07:00
epriestley
83617073f5 Rename TypehaeadUserParameterizedDatasource to PeopleUserFunctionDatasource
Summary: Ref T4100. This improves application scoping (Typeahead -> People) and uses a more consitent and concise name (Parameterized -> Function).

Test Plan:
  - `grep`
  - Used affected datasource.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4100

Differential Revision: https://secure.phabricator.com/D12532
2015-04-23 11:49:34 -07:00
epriestley
3a2c2ae3c3 Implement a scope selector for the global search
Summary:
See M1433. Fixes T7266. Fixes T4475. Ref T7314.

Future work/notes/etc:

  - Write the User Guide (see TODO).
  - This might needs some design tweaks -- I think it's functionally almost-equivalent to the mock, but the UI isn't quite the same.
  - (Mobile design is a touch off-looking I think?)
  - When you use a custom query, the duplicate "magnifying glass" icons are a little weird. Maybe change one or the other.
  - Maybe worth adding an "Open Documents in Current Application" option? Planning to wait for feedback on that.
  - Need a Quicksand integration to change the current application at some point.
  - Searching in "Current Application" from, e.g., the 404 page just searches all documents. Current plan is to just document this behavior, since the icon is a pretty good callout and it seems plausible that this is intuitive enough that users won't have a hard time with it.

Test Plan:
New dropdown:

{F379150}

Device-ish:

{F379151}

Normal search (current application, from maniphest, selects tasks):

{F379153}

Application search from non-application:

{F379154}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: johnny-bit, epriestley

Maniphest Tasks: T7266, T7314, T4475

Differential Revision: https://secure.phabricator.com/D12509
2015-04-22 14:31:36 -07:00
epriestley
4dea152215 Fix a possible undefined variable
Summary: Fixes T7881.

Test Plan: Inspection.

Reviewers: btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Maniphest Tasks: T7881

Differential Revision: https://secure.phabricator.com/D12500
2015-04-21 19:28:43 -07:00
epriestley
c509e80a74 Fix an issue where versus diffs tried to load invalid changeset IDs
Auditors: btrahan
2015-04-21 15:38:52 -07:00
epriestley
1858d2aa66 Make Differential timeline aware of ghostly inlines
Summary:
Ref T7447. Ref T7870.

When a ghostly inline appears on the page, the timeline isn't currently aware that it's present, so it links elsewhere.

Instead, apply adjustments before rendering the timeline.

Ref T5030. This makes the behaviors in T5030 irrelevant most of the time.

Test Plan:
Before:

{F378106}

After:

  - Inlines visible on page are linked directly.
  - Inlines which we can't port (e.g., on files not present on current page) are still linked off-page.
  - Used "Show Older" to page through and verify consistent rendering.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T5030, T7870, T7447

Differential Revision: https://secure.phabricator.com/D12497
2015-04-21 15:32:23 -07:00
epriestley
d8bd3efa2c Simplify timeline rendering on Differential revisions
Summary: Ref T7447. Remove the `loadInlineComments()` by-ref side-effect mess and make this load more direct.

Test Plan:
  - Viewed a revision.
  - Clicked "Show Older".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D12496
2015-04-21 15:31:43 -07:00
epriestley
a4a65bd5f1 Let ghostly inlines automatically expand changesets in enormous diffs
Summary:
Ref T7447. Ref T7870. When a diff affects more than 100 files, we collapse files by default, then expand files with inlines.

Ghostly inlines currently don't cause files to expand, but reasonably should.

Test Plan:
Before:

{F378065}

After:

{F378066}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T7447, T7870

Differential Revision: https://secure.phabricator.com/D12495
2015-04-21 15:31:18 -07:00
epriestley
8c39885076 Track ghostly inlines across file moves
Summary:
Ref T7447. Ref T7870.

  - Forward: When rendering some file "B" which was moved from "A", port ghosts for both "A" and "B" to it.
  - Backward: When porting a comment on "X" which was moved from some "Y", allow it to port to a file named "Y" if it can't find a file named "X".

Test Plan:
Before:

{F377809}

After:

{F377806}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7447, T7870

Differential Revision: https://secure.phabricator.com/D12493
2015-04-21 11:06:44 -07:00
epriestley
84f62bdf9a Move ported inlines which appear on nonvisible lines backward so they show up
Summary:
Ref T7447. Ref T7870. See T7870 for a detailed description of this issue.

NOTE: Replying to these inlines from the UI still does the wrong thing, because we use the database line number, not the UI line number.

Test Plan:
Before:

{F377732}

After:

{F377733}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7447, T7870

Differential Revision: https://secure.phabricator.com/D12491
2015-04-21 11:06:44 -07:00
epriestley
fe774e68e2 Distinguish between ported-forward and ported-backward comments
Summary:
Ref T7447. This might be overkill, but I want to over-explain things until we have more confidence that this is rarely confusing.

NOTE: I'm playing it a bit fast and loose with `setIsGhost()` (passing a dictionary) because making API changes requires changing the interface and Diffusion, which is a pain. I'll clean this up at the end once the interface is more final. This is well-contained for now.

Test Plan:
  - Viewed "base vs 2" in a diff with 3 diffs, saw some "older comments" and some "newer comments".
  - Hovered the tags for an explanation of comment spookiness.

{F377703}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D12490
2015-04-21 11:06:44 -07:00
epriestley
aa04e97de7 Prefer left-side for old comments in new-vs-new diff of diffs
Summary: Ref T7447. Ref T7870.

Test Plan:
Before:

{F377691}

After:

{F377692}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7870, T7447

Differential Revision: https://secure.phabricator.com/D12489
2015-04-21 11:06:44 -07:00
epriestley
b74bb0019e Filter nonvisible inlines out of changeset inline result set
Summary:
Ref T7447. After compiling inlines which will appear on the changeset, remove inlines which

Later stages remove these anyway, so it doesn't change anything to keep them around, but we can filter them out here cheaply.

This will also let us drive the Differential timeline view with the same logic a few diffs from now, to improve how it renders inlines. Generalize things a little bit.

Test Plan:
  - Made a comment on the left of diff 1.
  - Made diff 2.
  - Viewed diff 2 vs diff 1.
  - Verified old-left comment was filtered out by the new loop.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D12488
2015-04-21 11:06:44 -07:00
epriestley
b2d280ff51 Port comments through time and space in the common/best case
Summary:
Ref T7447. This ports comments forward and backward in the best case:

  - The old comment is on a changeset with the same filename.
  - The old and new files are pretty much the same, line-for-line.

This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice.

Errata:

  - Design is me cobbling something together, probably worth tweaking.
  - "Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff.

Test Plan: {F377214}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: johnny-bit, yelirekim, epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D12484
2015-04-21 11:06:44 -07:00
epriestley
5645a07d99 Modernize DifferentialInlineCommentQuery
Summary:
Ref T7447. This class is currently a big mess with a lot of `withWeirdSpecialThingUsedInOnePlace()` type qualifiers.

Try to generalize/normalize it a bit.

Test Plan:
- Viewed inline comments.
- Created a new inline comment.
- Edited an inline comment.
- Marked an inline comment complete.
- Deleted, then undeleted an inline comment.
- Previewed inline comments.
- Viewed drafts as another user, verified they don't show up.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D12483
2015-04-21 11:06:44 -07:00
epriestley
b85f8b91de Implement readProjectsFromRequest() helper for SearchEngines
Summary:
Ref T4100. This just makes the "specify stuff in query parameters" workflow a little better:

  - You can now do `?projects=differential,diffusion`.
  - You can now do `?projects=projects(alincoln)`.

Test Plan: Did that stuff ^^^^

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4100

Differential Revision: https://secure.phabricator.com/D12468
2015-04-20 10:06:22 -07:00
epriestley
4005bff567 Implement function-driven logical project queries in Differential
Summary:
Ref T4100. Ref T5595. This implements these fields in one mega-field:

  - Projects
  - Not in projects
  - In any project
  - Include results in no projects
  - In users' projects

Hopefully, this is a step in the right direction.

Test Plan: {F375555}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, chad, epriestley

Maniphest Tasks: T4100, T5595

Differential Revision: https://secure.phabricator.com/D12463
2015-04-20 10:06:21 -07:00
epriestley
a11dab59b0 Require TokenizerControl to always have a datasource
Summary:
Ref T4100. I can simplify the logic a bit here by moving some rendering into the datasources, but a few TokenizerControls currently don't have datasources.

Require datasources and always provide datasources.

Test Plan:
  - Used previously-datasourceless controls (e.g., "Add Reviewers").
  - Used normal controls.
  - Manually verified that no other controls are missing datasources.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4100

Differential Revision: https://secure.phabricator.com/D12456
2015-04-20 10:06:13 -07:00
epriestley
f5580c7a08 Make buildWhereClause() a method of AphrontCursorPagedPolicyAwareQuery
Summary:
Ref T4100. Ref T5595.

To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.

With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.

For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.

For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.

This causes no behavioral changes.

Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, hach-que, epriestley

Maniphest Tasks: T4100, T5595

Differential Revision: https://secure.phabricator.com/D12453
2015-04-20 10:06:09 -07:00
epriestley
ea97d75a67 Make Differential use viewer's token instead of viewer() token again
Summary: Ref T4100. This restores the simpler behavior. See discussion in T4100#107445

Test Plan: Used Differential search, saw my token.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4100

Differential Revision: https://secure.phabricator.com/D12447
2015-04-17 11:06:58 -07:00
epriestley
845466b49b Implement viewer() and members(project) typeahead functions
Summary:
Ref T4100. This is still a bit rough around the edges, but mostly does what we're after.

  - Implements viewer() and members(...) functions.
  - The new browse workflow makes these discoverable.

Test Plan: {F374201}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: chad, epriestley

Maniphest Tasks: T4100

Differential Revision: https://secure.phabricator.com/D12444
2015-04-17 11:06:58 -07:00
epriestley
156b156e77 Give Conduit params/return/errors protected visibility
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.

Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.

Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.

Test Plan:
  - Ran `arc unit --everything`.
  - Called `conduit.query`.
  - Browsed Conduit UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: hach-que, epriestley

Maniphest Tasks: T5873, T7803

Differential Revision: https://secure.phabricator.com/D12380
2015-04-13 11:58:35 -07:00
epriestley
2794c69db5 Remove getPagingColumn() / getReversePaging()
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.

Test Plan: Sorted and paged results in various applications.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12378
2015-04-13 11:58:32 -07:00
epriestley
4114560844 Modernize more paging/order queries
Summary:
Ref T7803. Removes some getReversePaging().

This also fixes `null` column handling, by adding an explicit `'null'` key with possible values "head" (put NULL before other values) or "tail" (put NULL after other values).

Maniphest has some glitchiness in paging through NULLs right now, but I believe it's all pre-existing and will be resolved when it fully converts. Diffusion is fully converted and pages through NULL correctly.

Test Plan:
  - Failed to identify any reason for ChangesetQuery to reverse paging.
  - Paged thorugh Diffusion.
  - Paged through Maniphest.
    - Maniphest has some issues when paging inside a NULL section, but these issues are preexisting and will be resolved later in this change sequence.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12371
2015-04-13 11:58:30 -07:00
epriestley
51dabc5007 Modernize Differential paging/ordering
Summary: Ref T7803. Move Differential off getPagingColumn() / getReversePaging().

Test Plan: Paged Differential results.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12362
2015-04-13 11:58:28 -07:00
epriestley
9b5198f463 Remove ORDER_PATH_MODIFIED from Differential
Summary:
Ref T7803. This is a performance hack, not a real order, and isn't really meaningful or pageable.

After D12158, we constraint his query on `dateModified` anyway, which should generally give the database a relatively small result set to examine.

Test Plan: Browsed Differential and Diffusion. Checked query plan, it didn't look too crazy.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12361
2015-04-13 11:58:26 -07:00
epriestley
a4a198342e Modernize ReleephProjectQuery ordering/paging
Summary: Ref T7803. Continue removing implementations of getPagingColumn() and getReversePaging().

Test Plan: Browsed and paged through Releeph projects, Maniphest tasks, Diffusion repositories.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12358
2015-04-13 11:58:21 -07:00
epriestley
604d1409f1 Make buildPagingClauseFromMultipleColumns() safer
Summary: Ref T7803. Reduce the amount of code we're trusting to build SQL queries.

Test Plan:
  - Paged through results in Maniphest, Differential and Diffusion.
  - Some of the NULLable groups in Maniphest are a bit funky but this was preexisting.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12353
2015-04-13 11:58:15 -07:00
epriestley
d44f05eead Clean up some more handle callsites
Summary: Ref T7689.

Test Plan:
- Viewed Calendar browse and detail views.
- Went through Differential update workflow.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7689

Differential Revision: https://secure.phabricator.com/D12341
2015-04-09 16:15:13 -07:00
epriestley
b16db61a87 Allow "send me an email" in personal rules to punch through settings
Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.

In particular, this is stronger than these downgrades:

  - Downgrades due to "self actions";
  - downgrades due to "mail tags".

Test Plan:
  - Wrote various Herald rules with "Send me an email" rules.
  - Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
  - Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7731

Differential Revision: https://secure.phabricator.com/D12300
2015-04-06 10:01:32 -07:00
epriestley
63f4e66b11 Attach HeraldRules to HeraldEffects
Summary:
Ref T7731. For no particular reason, we currently put `ruleID` and `rulePHID` on `HeraldEffect` objects.

Pretty much all callers need the `HeraldRule` objects instead, and some go to great lengths to get them.

Just attach the `Rule` objects.

Test Plan: Will test thoroughly after next-ish changeset.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7731

Differential Revision: https://secure.phabricator.com/D12269
2015-04-06 10:00:45 -07:00
epriestley
a40748a46c Lift handling of Herald "email" effect to Adapter
Summary:
Ref T7731. Every adapter subclass currently implements this effect in an essentially identical way.

Some day far from now the effects will be modular and this mess will vanish completely, but reduce its sprawl for now.

Test Plan: I'll test this thoroughly at the end of the change sequence since writing rules is a pain.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7731

Differential Revision: https://secure.phabricator.com/D12268
2015-04-06 10:00:35 -07:00
Joshua Spence
ea376685ae Fix some odd looking arrays
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12281
2015-04-05 22:29:39 +10:00
epriestley
eb81fd1562 Expose all application mail receivers
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.

  - Reduce code duplication between existing receivers.
  - Expose receiving objects in help menus where appropriate.
  - Connect some "TODO" receivers.

Test Plan:
  - Sent mail to every supported object type.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12249
2015-04-01 11:52:02 -07:00
epriestley
25f8a37f85 Provide better general high-level documentation on mail commands
Summary: Ref T7199. This makes the page look less janky and provides more context about how mail commands work and how to use them.

Test Plan: {F355959}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12245
2015-04-01 11:51:49 -07:00
epriestley
c169199e64 Allow applications to have multiple "help" menu items
Summary:
Ref T7199. Convert the single help menu item into a dropdown and allow applications to list multiple items there.

When an application has mail command objects, link them in the menu.

Test Plan:
{F355925}

{F355926}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12244
2015-04-01 11:51:48 -07:00
epriestley
6f95b325c6 Generate mail command documentation from active commands
Summary: Ref T7199. This needs some polish and isn't reachable from the UI, but technically has all of the information.

Test Plan:
{F355899}

{F355900}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12241
2015-04-01 11:51:47 -07:00
epriestley
d4bfaa2feb Move Differential to modular mail commands
Summary: Ref T7199. Convert Differential to modern modular commands.

Test Plan: Used `bin/mail receive-test` to send command and comment mail to Differential.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12239
2015-04-01 08:40:12 -07:00
epriestley
34db543d27 Remove all application-specific reply handler class overrides
Summary:
Ref T7199. In the vein of D12231, these options were a bad idea.

  - They once served a very narrow, Facebook-specific need (see T1992), except even Facebook only used the Differential setting AFAIK.
  - Outside of that special case, they are unused and essentially unusable (generally speaking, they do not meaningfully implement anything modular or replaceable).
  - I have no knowledge of any install ever changing these settings, and can imagine no reason why they would.

Moving forward:

  - If they really need to, they can fork locally and chagne one line.
  - I expect "!actions" to make mail at least somewhat more modular soon, anyway.
  - Any derived handlers would break after T7199 and need to be rewritten anyway, so this is just taking advantage of a BC break to do cleanup.

Test Plan:
  - Grepped for removed configuration.
  - Sent some mail from applications, verified the reply handlers set proper reply addresses.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12233
2015-03-31 17:22:01 -07:00
epriestley
bad645f1ec Remove all application-specific reply handler domains
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.

  - There is zero reason to ever set these to different things.
  - Unsurprisingly, I don't know of any install which has them set to different things.

Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.

We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.

Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12231
2015-03-31 16:48:40 -07:00
epriestley
0d99c84bd7 Modernize email command parsing
Summary:
Ref T7199. This prepares for an exciting new world of more powerful "!action" commands. In particular:

  - We parse multiple commands per mail.
  - We parse command arguments (these are currently not used).
  - We parse commands at the beginning or end of mail.

Additionally:

  - Do a quick modernization pass on all handlers.
  - Break legacy compatibility with really hacky Facebook stuff (see T1992). They've theoretically been on notice for a year and a half, and their setup relies on calling very old reply handler APIs directly.
  - Some of these handlers had some copy/paste fluff.
  - The Releeph handler is unreachable, but fix it //in theory//.

Test Plan:
- Sent mail to a file; used "!unsubscribe".
- Sent mail to a legalpad document; used "!unsubscribe".
- Sent mail to a task; used various "!close", "!claim", "!assign", etc.
- Sent mail to a paste.
- Sent mail to a revision; used various "!reject", "!claim", etc.
- Tried to send mail to a pull request but it's not actually reachable.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12230
2015-03-31 16:48:27 -07:00
epriestley
030e05aa4c Remove reply handler instructions from email
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.

Throwing it away makes it easier to move forward and lets us get rid of a config option.

This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:

  +---+
  | O |
  +---+---------------------+
  | Maniphest Documentation |
  | Maniphest Email Actions |
  +-------------------------+

Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.

Test Plan: Grepped for `replyHandlerInstructions`, got no hits.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12229
2015-03-31 16:48:17 -07:00
epriestley
d403700e1f Convert all tokenizers to take token/scalar inputs
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.

Test Plan:
- Sent a new message; used "To".
  - I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
  - OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^

Did not test:

- Lint is nontrivial to test locally, I'll test it in production.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4100, T7689

Differential Revision: https://secure.phabricator.com/D12224
2015-03-31 14:10:55 -07:00
epriestley
0f52fc771d Remove implode_selected_handle_links()
Summary: Ref T7689. Use the newer, less-janky stuff for rendering handles.

Test Plan:
- Viewed a revision hovercard; viewed "Author", viewed "Reviewers", viewed "Tasks".
- Viewed a task hovercard; viewed "Assigned To"; viewed "Projects"; viewed other edge fields.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7689

Differential Revision: https://secure.phabricator.com/D12209
2015-03-31 05:48:20 -07:00
epriestley
a17542ab28 Touch up PHP/JS interactions for inline comments
Summary:
Ref T1460. Overall:

  - Pass `objectOwnerPHID` consistently.
  - Pass viewer consistently.
  - Set the correct draft state for checkboxes on the client.

Test Plan:
  - Made inline comments in Differential.
  - Made inline comments in Diffusion.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12186
2015-03-27 17:08:31 -07:00
epriestley
174cf82398 Provide getObjectOwnerPHID() on inline comment views
Summary:
This returns the PHID of the current revision owner, or the commit author, if one exists.

NOTE: For drafts, we currently return `null`; I'll fix that in a future change. Should be correct for submitted comments.

Test Plan: Added an inline, nothing seemed broken.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12185
2015-03-27 11:23:10 -07:00
epriestley
cce6d06fa5 Move abandoned revisions to "needs review" when updated
Summary:
Fixes T7602. This is similar to the existing behavior for "changes planned" and "needs revision" revisions.

Also fix the "Update Diff" workflow so it correctly selects closed revisions as attachable.

Test Plan: Updated an abandoned revision, saw it change to "Needs Review".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7602

Differential Revision: https://secure.phabricator.com/D12167
2015-03-26 11:11:33 -07:00
epriestley
e5445de163 Show only recent open revisions affecting the same files
Summary: Fixes T5658. Over a long period of time, some cruft can build up here. Only show revisions which have been updated in the last 30 days.

Test Plan:
  - Viewed panel in Differential and Diffusion.
  - Changed limit from 30 days to 30 seconds and saw no revisions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5658

Differential Revision: https://secure.phabricator.com/D12158
2015-03-25 10:21:56 -07:00
epriestley
17e1e7a65a Document the need to purge caches after updating differential.generated-paths
Summary: Fixes T6378.

Test Plan: Set config to `/.*/`, created a new diff, everything was collapsed as generated.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T6378

Differential Revision: https://secure.phabricator.com/D12159
2015-03-25 07:29:09 -07:00
epriestley
aa310230b6 Detect moves and copies with some unchanged lines as moves or copies
Summary:
Ref T1266. We won't detect a move/copy if fewer than 3 lines are changed.

However, you may move a block like:

  Complicated Line A
  Trivial Line B
  Complicated Line C

...where "Trivial Line B" is something like a curly brace. If you move this block somewhere that happened to previously have a similar trivial curly brace line, we won't be able to find 3 contiguous added lines in order to detect the copy/move.

Instead, consider both changed and unchanged lines when trying to find contiguous blocks. This allows us to detect across gaps where lines were not actually changed.

This new algorithm may be too liberal (for example, we may end up incorrectly identifying moved/copied code before or after changed lines, not just between changed lines), but we can keep an eye on it and tweak it. The algorithm is better factored and better covered, now.

Test Plan:
  - Added a unit test for this case.
  - Spot-checked a handful of diffs and generally saw behavior that made sense and looked better than before.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1266

Differential Revision: https://secure.phabricator.com/D12146
2015-03-24 13:12:24 -07:00
epriestley
373aaa643a Clean up copy detection code a bit
Summary:
Ref T1266. This doesn't change any behaviors, but some of this code has a lot of really complicated conditionals and I tried to break that up a bit.

Also, reexpress this stuff in terms of the "structured" parser in D12144.

Test Plan: Unit tests still pass. They aren't hugely comprehensive but did reliably fail when I screwed stuff up.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1266

Differential Revision: https://secure.phabricator.com/D12145
2015-03-24 13:12:09 -07:00
epriestley
74a4c2cf0b Provide better parsing primitives for hunks
Summary:
Ref T1266. This prepares to fix case (2) on T1266 by improving the robustness of hunk parsing.

In particular, the copy detection code abuses this API because it isn't currently expressive or flexible enough.

Make it more flexible and cover it exhaustively.

I'll move callsites to the new stuff in upcoming revisions.

Test Plan: Unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1266

Differential Revision: https://secure.phabricator.com/D12144
2015-03-24 13:11:37 -07:00
epriestley
21826ed7b3 Don't highlight very large files by default
Summary:
Ref T5644. See some discussion in D8040.

When a file is very large (more than 64KB of text), don't activate syntax highlighting by default. This should prevent us from wasting resources running `pygmentize` on enormous files.

Users who want the file highlighted can still select "Highlight As...".

The tricky part of this diff is separating the headers into "changeset" headers and "undershield" (rendering) headers. Specifically, a file might have these headers/shields:

  - "This file is newly added."
  - "This file is generated. Show Changes"
  - "Highlighting is disabled for this large file."

In this case, I want the user to see "added" and "generated" when they load the page, and only see "highlighting disabled" after they click "Show Changes". So there are several categories:

  - "Changeset" headers, which discuss the changeset as a whole (binary file, image file, moved, added, deleted, etc.)
  - "Property" headers, which describe metadata changes (not relevant here).
  - "Shields", which hide files from view by default.
  - "Undershield" headers, which provide rendering information that is only relevant if there is no shield on the file.

Test Plan:
  - Viewed a diff with the library map, clicked "show changes", got a "highlighting disabled" header back with highlighting disabled.
  - Enabled highlighting explicitly (this currently restores the shield, which it probably shouldn't, but that feels out of scope for this change). The deshielded file is highlighted per the user's request.
  - Loaded context on normal files.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T5644

Differential Revision: https://secure.phabricator.com/D12132
2015-03-24 05:26:39 -07:00
epriestley
dd3afe2aa2 Lift inline comment state transactions into core (in Differential)
Summary: Ref T1460. Follows D12129 and reduces code duplication.

Test Plan: Changed inline state in Differential.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12130
2015-03-24 05:26:16 -07:00
epriestley
9f3210c883 Publish draft "done" status when submitting comments/updates/actions/inlines
Summary:
Ref T1460. When a revision author updates/comments/etc on a revision, publish all their checkmarks.

This doesn't handle Diffusion/audits yet.

Test Plan: {F346870}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12126
2015-03-24 05:26:12 -07:00
epriestley
4310c4ed53 Track a "Done" state on inline comments
Summary:
Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc.

Specifically, these are the behaviors:

  - You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider..").
  - You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff.
  - When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold:
    - Be consistent with how inlines work.
    - Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories.
  - The actual bit where done-ness publishes isn't implemented.
  - UI is bare bones.
  - No integration with the rest of the UI yet.

Test Plan: Clicked some checkboxes.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: paulshen, chasemp, epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12033
2015-03-24 05:26:11 -07:00
epriestley
d0b3f199bb Provide a smoother "update diff" web workflow
Summary:
Fixes T1102. If you don't use `arc`, the web workflow requires some extra needless steps when updating diffs.

Provide a more streamlined "Update Diff" workflow.

Test Plan: {F347750}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1102

Differential Revision: https://secure.phabricator.com/D12131
2015-03-23 10:44:33 -07:00
epriestley
5001aadf46 Throw a more helpful error for bad Differential actions
Summary: Ref T7611. This should let us figure out the root cause, hopefully.

Test Plan: iiam

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7611

Differential Revision: https://secure.phabricator.com/D12124
2015-03-23 09:10:18 -07:00
epriestley
dd501117e8 When deleting inline comments, offer "undo" instead of prompting
Summary:
Ref T2009. Ref T1460.

Fixes T2618. When users hit "Delete" on inline comments, delete immediately and offer them "Undo". If they delete indirectly (e.g., by clicking "Delete" from the preview at the bottom of the page), we still prompt them, because the "Undo" action either won't be available or may not be easy to find. This is a "refdelete".

Fixes T6464. This was just a mess. Make it not as much of a mess. It should work now. Pretty sure.

Fixes T4999. We did not refresh these links often enough to find targets for them, so they could race with content. Reevaluate them after loading new changes.

Test Plan:
  - Deleted and undid deletion of inlines from main view and preview.
  - Clicked "View" on inlines.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6464, T4999, T2618, T1460, T2009

Differential Revision: https://secure.phabricator.com/D12032
2015-03-09 17:27:51 -07:00
epriestley
7427a6e648 Extend TransactionCommentQuery for Differential
Summary: Ref T2009. Ref T1460. Replace hard-coded garbage with a real Query-layer query.

Test Plan: Submitted inline comments in Differential.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009, T1460

Differential Revision: https://secure.phabricator.com/D12027
2015-03-09 14:11:20 -07:00
epriestley
4d86d51125 Prepare TransactionCommentQuery for extension
Summary:
Ref T2009. Ref T1460. The way Diffusion and Differential load inlines is horrible garbage right now:

  - Differential does an ad-hoc query to get the PHIDs, then does a real load to policy check.
  - Diffusion completely fakes things. In practice this is not a policy violation, but it's dangerous.

Make TransactionCommentQuery extensible so we can subclass it and get the query building correctly in the right Query layer.

Specifically, the Diffusion and Differential subclasses of this Query will add appropriate `withX()` methods to let us express the query in SQL.

Test Plan: Loaded, previewed, edited, and submitted inlines in Differential and Diffusion

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009, T1460

Differential Revision: https://secure.phabricator.com/D12026
2015-03-09 14:11:18 -07:00
epriestley
2972894a4d Write "hasReplies" to database for inline comments
Summary:
Ref T1460. Ref T2618.

When publishing a draft inline, mark the inline it replies to (if any) as replied to.

Also, don't load deleted comments as drafts (sets the stage for T2618).

I'll make an effort to clean up the loading mess here in the next revision, and find some more appropriate home for the shared code.

Test Plan: Made and replied to comments in Differential and Diffusion. Saw comments get marked as "Has Replies" and "Is Reply".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2618, T1460

Differential Revision: https://secure.phabricator.com/D12025
2015-03-09 14:11:16 -07:00
epriestley
f1f2c5d01d Reduce code duplication in inline right/left side tracking
Summary: Ref T2009. These subclasses have a mixture of similar methods, move them all to the base class.

Test Plan: Created/edited/undo/submitted comments on the left and right sides of a diff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D12024
2015-03-09 12:53:40 -07:00
epriestley
10a84d66db Show a more tailored message when adding empty files
Summary: Fixes T6695. Tailor this message.

Test Plan: {F333305}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T6695

Differential Revision: https://secure.phabricator.com/D12022
2015-03-09 10:27:18 -07:00
epriestley
b7be4ccca1 Hide revision content if author has not signed required legal documents
Summary: Fixes T7446.

Test Plan: {F333301}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7446

Differential Revision: https://secure.phabricator.com/D12021
2015-03-09 10:26:54 -07:00
epriestley
56a9709008 Reduce code duplication for inline "Undo"
Summary:
Ref T2009. This is another almost-identical copy of the row scaffolding, which has the same 1up/2up bugs as the 8 other copies of this code.

Turn the "undo" element into an InlineCommentView so we can scaffold it.

Then, scaffold it with the same code as everything else.

Test Plan: Hit "Undo", swapped from 1up to 2up, hit "undo" again, swapped back, tried left/right, everything rendered with proper scaffolding.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D12019
2015-03-09 10:26:53 -07:00
epriestley
082b7f95e6 Explicitly track inline comment reply threading
Summary:
Ref T1460. Track and store which comments are threaded replies to other comments, vs merely appearing on the same lines.

This doesn't actually write `hasReplies` yet, since that needs to happen when we un-draft comments on submission.

Test Plan: Made inline comments in Differential and Diffusion, including replies. Replies were marked as "Is Reply".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12017
2015-03-09 10:26:50 -07:00
epriestley
c35cafda2c Use ChangesetListView on Differential standalone view
Summary:
Fixes T4452. Ref T2009. There's a hierarchy of changeset rendering power: only low-level calls, use of ChangesetDetailView, then use of ChangesetListView (a list of DetailViews).

Prior to work here, the various changeset rendering controllers got their hands dirty to varying degrees, with some using only the lowest-level rendering pipeline:

  - Phriction: no view (lowest level)
  - Diffusion: DetailView
  - Differential Changeset: DetailView
  - Differential Diff: ListView
  - Differential Revision: ListView

I brought Phriction up to use DetailView, but want to bring everything all the way up to use ListView. Each composition layer adds more features to diff browsing. In particular, this change enables "Highlight As", switching 1up vs 2up, adding inlines, etc., on the standalone view.

Test Plan:
  - Viewed a changeset standalone. Could change highlighting, switch 1up vs 2up, add and edit inlines, etc.
  - Viewed a revision; no behavioral changes.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4452, T2009

Differential Revision: https://secure.phabricator.com/D12012
2015-03-09 10:26:47 -07:00
Chad Little
076cc6ed7e Change setErrorView to setInfoView in PHUIObjectBoxView
Summary: Renames the method in PHUIObjectBoxView to match the new PHUIInfoView class.

Test Plan: grepped codebase. Went to Calendar and tried a new status.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12005
2015-03-06 17:03:18 -08:00
epriestley
68fa70eacb Make images work in the unified diff view
Summary: Ref T2009. Still a touch glitch-ish but essentially functional now.

Test Plan: Viewed image diffs in 1up and 2up views. Made inline comments on them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D12003
2015-03-06 15:03:38 -08:00
epriestley
f9cb366f00 Remove duplicate inline scaffold in 2up renderer
Summary: Ref T2009. Remove the 4 (!!) copies of this code.

Test Plan:
  - Added, edited, and removed inline comments in 2up view.
  - Stacked a bunch of comments on the same line and saw the JS place them correctly.
  - Created an image diff and added, edited and removed inlines on it.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D12000
2015-03-06 15:00:43 -08:00
epriestley
ac60b23ef9 Remove duplicate inline row scaffold in 1up renderer
Summary: Ref T2009. This can now be removed.

Test Plan: Added, edited and deleted an inline comment in 1up view.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11998
2015-03-06 15:00:39 -08:00
epriestley
1352be827e Begin separating inline comment scaffolding from other renderers
Summary:
Ref T2009. Inline comments have "scaffolding", which is basically some empty table cells/rows around them to get the layout correct.

The scaffolding depends on the renderer, since the cells are different for side-by-side vs unified diffs.

This is currently duplicated all over the place:

  - Edit view has 1up/2up.
  - Detail view has 1up/2up.
  - 1up renderer has 1up.
  - 2up renderer has four separate copies of the 2up logic.

These all have subtle differences, which are mostly bugs. Start making the scaffolding more composable so we can get rid of that mess.

Test Plan: Added, edited, and removed inline comments on unified and side-by-side diffs.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11997
2015-03-06 15:00:33 -08:00
epriestley
1088d34e58 Rename inline comment views to "PHUIDiff" and give them a base class
Summary:
Ref T2009. These classes are "Differential" now, but are used elsewhere in diff infrastructure (e.g., Diffusion).

  - Rename them to "PHUIDiff".
  - Move them to "src/infrastructure/".
  - Give them a base class.

Test Plan: Interacted with inlines in unified and side-by-side views.

Reviewers: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11996
2015-03-06 15:00:14 -08:00
epriestley
0af80c1d90 Further improve line grouping in unified views
Summary:
Ref T2009. This tweaks things a bit more to improve consecuitive groups of added and removed lines.

Generally, it gives us "old, old, old, new, new, new" intead of "old, new, old, new, old, new".

Feelin' real good about having unit tests for this stuff.

Test Plan: Unit tests, looked at diffs in web UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11994
2015-03-06 09:58:26 -08:00
epriestley
b3d1ecebc7 Fix another issue with line order on unified diffs
Summary: This improves some cases with interleaved added and removed lines, and adds test coverage.

Test Plan:
  - Added and executed unit tests.
  - Viewed raw diff and saw sensible/expected output.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D11992
2015-03-05 16:24:04 -08:00
epriestley
9564b0a40e Improve behavior of inline rendering with unified views
Summary:
Ref T2009. This reduces how buggy inlines are. They're still buggy.

Specifically, the inline endpoint didn't know how to scaffold inlines before, so some of them ended up rendering in the wrong rows or breaking layouts.

This passes the current renderer through to the inline editor endpoint, so it can at least get the layout correct.

Test Plan: Interacted with inlines in unified and side-by-side views.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11988
2015-03-05 14:11:51 -08:00
epriestley
bd5985e67d Add "Copy" and "Coverage" columns to unified view
Summary:
These aren't being populated yet; they mostly fix some JS errors with inlines.

For example, the inline hover reticle relies on adjusting its width to account for the "copy" column, and failed when the column did not exist.

Test Plan:
  - Hovering inlines in unified now works, mostly.
  - Interacted with inlines in side-by-side.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D11985
2015-03-05 14:11:36 -08:00
epriestley
a8c9f2e8b0 Fix an issue where inlines appear in the wrong place in one-up views
Summary: Ref T2009. Unchanged lines should always go above inlines; we get nonsense results otherwise.

Test Plan: Inline now shows in correct place in unified view.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11987
2015-03-05 14:04:21 -08:00
epriestley
35c1dbf1f8 Unify changeset line ID rendering and bring it to unified diffs
Summary:
Ref T2009. Currently, lines don't get their "C123NL456" IDs set in the unified view. This is the major way that inlines are glued to changesets.

Simplify this rendering and bring it into the HTML renderer, then use it in the OneUp renderer.

Test Plan:
  - Interacted with side-by-side inlines (hovered, added, edited, deleted), saw unchanged behavior.
  - Interacted with unified inlines. They still don't work, but the error that breaks them is deeper in the stack.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11983
2015-03-05 14:03:57 -08:00
epriestley
cb886eb60e Share code for context link rendering on side-by-side diffs
Summary: Ref T2009. I've clicked these links like 200 times in testing now, so I'm feeling pretty good about them.

Test Plan: Viewed links in side-by-side diff, clicked them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11981
2015-03-05 14:03:26 -08:00
epriestley
aa98d9e5f8 Merge behavior-show-more into behavior-populate
Summary: Ref T2009. It doesn't make sense to have these as separate behaviors. We require a ChangesetViewManager to track view parameter state.

Test Plan: Interacted with changesets in Phriction, Differential and Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11979
2015-03-05 14:03:15 -08:00
epriestley
ad3c94dd45 Make "Show Context" persist rendering, whitespace, encoding, etc
Summary:
Ref T2009. Currently, we do not persist view parameters when making context rendering requests.

The big one is the renderer (1up vs 2up). This makes context on unified diffs come in with too many columns.

However, it impacts other parameters too. For example, at HEAD, if you change highlighting to "rainbow" and then load more context, the context uses the original highlighter instead of the rainbow highlighter.

This moves context loads into ChangesetViewManager, which maintains view parameters and can provide them correctly.

  - This removes "ref"; it is no longer required, as the ChangesetViewManager tracks it.
  - This removes URI management from `behavior-show-more`; it is no longer required, since the ChangesetViewManager knows how to render.
  - This removes "whitespace" since this is handled properly by the view manager.

Test Plan:
  - Used "Show Top" / "Show All" / "Show Bottom" in 1-up and 2-up views.
  - Changed file highlighting to rainbow, loaded stuff, saw rainbow stick.
  - Used "Show Entire File" in 1-up and 2-up views.
    - Saw loading chrome.
    - No loading chrome normally.
  - Made inlines, verified `copyRows()` code runs.
  - Poked around Diffusion -- it is missing some parameter handling, but works OK.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11977
2015-03-05 14:03:00 -08:00
epriestley
df661eca35 Always run "behavior-populate" before "behavior-show-more"
Summary:
Ref T2009. This clears the stage for D11977.

Specifically, D11977 moves "show context" logic into ChangesetViewManager, but those objects won't exist if we don't run "behavior-populate" first.

Generally, this increases consistency across changeset views -- which is still very low overall, but getting slightly better.

Both of these should probably move up more and use ChangesetListView, but we don't need to do that quite yet.

Test Plan:
  - Took changeset actions in Phriction diff view.
  - Took changeset actions in Differential standalone view.
  - Took changeset actions in normal Differential view.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11978
2015-03-05 14:02:45 -08:00