1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

1373 commits

Author SHA1 Message Date
epriestley
8f9b7f4196 Move Differential to proper subscriptions
Summary:
Ref T2222. Ref T4415. We're still writing Differential subscription stuff into this weird legacy `differential_relationship` table, which is like an edge table but extremely ancient.

Move it into a proper table.

I've removed `withSubscriptions()` from `DifferentialRevisionQuery`. It was weird, doesn't work consistently with other similar filters, and was only used by the API. Now it means "ccs", which is consistent with the ApplicationSearch UI and with Maniphest.

Test Plan:
Without migrating, added and removed subscribers via various workflows. Queried for subscribers. Everything worked as expected.

Ran the migration, verified data survived.

Reviewers: btrahan

Reviewed By: btrahan

CC: FacebookPOC, aran

Maniphest Tasks: T2222, T4415

Differential Revision: https://secure.phabricator.com/D8202
2014-02-12 08:53:40 -08:00
epriestley
1dbfc56d35 Write one DifferentialComment per CommentEditor action
Summary:
See D8200. Ref T2222. Instead of writing one comment which can have a ton of different effects, write a series of one-effect comments. These will be easier to convert into ApplicationTransactions.

This has a minor user-facing effect of making these multiple-action comments render separately:

{F111919}

Once the migration completes, they should automatically merge together nicely again.

Test Plan: Made a bunch of comments and took a bunch of actions, all of which worked normally except that they rendered as several things instead of just one.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, FacebookPOC

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8201
2014-02-11 15:21:21 -08:00
epriestley
c65fad3fca Separate revision updates into two separate comments
Summary:
Ref T2222. Instead of writing one comment which performs both a diff update and adds a comment, write two comments, one for each action. These will translate directly into ApplicationTransactions writes.

This has a small impact on the UX: these updates now render in two rows, instead of one. After T2222, they'll automerge back together.

{F111909}

Test Plan: Updated a revision.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8200
2014-02-11 15:21:14 -08:00
epriestley
8e232a8bf1 Allow Differential Mail to accept multiple comments for one mail
Summary:
Ref T2222. Currently, one `DifferentialComment` can do a lot of things (add ccs, reviewers, comments, inline comments, and perform state changes). In the future, each `ApplicationTransaction` does only one thing. This is the primary piece of complexity which makes the upcoming migration risky, because each comment needs to migrate into multiple transactions.

I want to mitigate this complexity as much as possible before the migration itself happens. One approach I'm going to use to do that is to start writing one comment per effect now, so the mapping is more direct when the migration itself happens and the write code can be straightforward (one row per save()) after the migration.

This tackles a small piece of that, which is the mail Differential sends. Currently, Differential mail acts on a single comment. Instead, allow it to act on a list of comments, but always give it one comment for now. In the future, we can hand it several comments instead and still get the expected behavior.

This change should have no impact on any application behaviors.

Test Plan:
  - Commented;
  - commented with inline;
  - added reviewers;
  - added CCs;
  - added CCs via mentions;
  - updated revision;
  - looked at all the mail, all of which seemed sane.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8199
2014-02-11 15:21:07 -08:00
epriestley
305fb3fbd9 Migrate all Differential comment text into new storage
Summary:
Ref T2222. Currently, `DifferentialComment` stores both (a) the text of comments and (b) various other transaction details. This data needs to map to both Transactions and TransactionComments in the long run. This diff separates out all the data which is bound for the TransactionComment table, so that when we migrate `DifferentialComment` itself it will //only// need to migrate into the Transactions table. This is a much simpler migration than the inline comment one was, partly because it set up infrastructure and partly because the data is less complex.

Basically, I'm just proxying the read/write for the comment text into the other table. All readers already go through the Query class, and there are only three writers (preview, comment, implicit comment on diff update) which are all highly regular and straightforward to test.

We can also back out of this diff very easily: doing double writes cost only one line of code (`$this->content = $content;`) so we have proper double writes and a trivial revert path.

Test Plan:
  - Without migrating, added comments and saw them show up.
  - Migrated.
  - Saw all the old comments, and no damage to the new ones.
  - Added new comments.
  - Used comment preview.
  - Updated a revision to implicitly create an update comment and verified it looked OK.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8196
2014-02-11 11:34:15 -08:00
Chad Little
ee44c6db53 Translate Differential Feed/Comment/Asana stories
Summary: I don't think this is too terrible, and makes the future easier? Maybe?

Test Plan: ALLCAPS translation, Viewed a diff, feed, and notifications.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8175
2014-02-09 10:07:29 -08:00
Bob Trahan
9be4df02c2 Herald - Add "new" field to herald
Summary: ...and surface it in all adapters except commit adapters. Values are true or false. Ref T4294

Test Plan: made a herald rule to be cc'd on new tasks. was cc'd on new tasks and not cc'd on updates to existing tasks.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4294

Differential Revision: https://secure.phabricator.com/D8142
2014-02-04 10:43:31 -10:00
epriestley
e7d671c8d3 Index "Closed" and "Unowned" relationships explicitly
Summary: Ref T4365. Two diffs from now, I'm changing the UI a bit to let you search for closed and unowned documents more explcitly. To support this in ElasticSearch and more easily in MySQL search, make these explicit, positive relationships.

Test Plan: `bin/search index --all`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4365

Differential Revision: https://secure.phabricator.com/D8122
2014-02-03 12:51:18 -08:00
Aviv Eyal
d0e30882a1 Add Disabled mode to landing revision ui
Summary:
Fixes T4066.

add `isActionDisabled()` to DifferentialLandingStrategy, which also explains why it is so.
Make an appropriate pop-up in the controller.

Also make the whole UI "workflow", and convert `createMenuItems()` to `createMenuItem()` (Singular).

Test Plan: Click "Land to..." button in all kinds of revisions.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4066

Differential Revision: https://secure.phabricator.com/D8105
2014-01-30 09:07:50 -08:00
epriestley
08225bd860 Remove policy caveat from Differential
Summary: Policies are now fully supported in Differential.

Test Plan: Grepped for other caveats, looks like I've already removed htem all.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8095
2014-01-29 11:44:10 -08:00
epriestley
8735ec4dbe Wrong method name.
Auditors: btrahan
2014-01-26 15:30:38 -08:00
epriestley
014a873773 Update DifferentialDiff: add repositoryPHID, drop parentRevisionID
Summary:
Moves away from ArcanistProjects:

  - Adds storage for diffs to be directly associated with a repository (instead of indirectly, through arcanist projects). Not really populated yet.
  - Drops `parentRevisionID`, which is obsoleted by the "Depends On" edge. This is not exposed in the UI anywhere and doesn't do anything. Resolves TODO.

Test Plan: Ran storage upgrades, browsed around, lots of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8072
2014-01-26 15:29:22 -08:00
Peng Li
c5c9cd415d Allow arc project and branch showing up in diff emails
Summary: Add the arc project and branch fields in emails for revisions under review. I am not quite sure why we only show them for changes which is already accepted or needs revision. It would be nice to have them for changes under review too.

Test Plan: Create a new revision and check email

Reviewers: epriestley, lifeihuang, JoelB, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8035
2014-01-22 09:12:05 -08:00
epriestley
9f35c7cc26 Complete modularization of the GC daemon
Summary: This modularizes the rest of the GC submethods. Turned out there was nothing tricky.

Test Plan: Ran `bin/phd debug garbage` and got reasonable looking behavior and output.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7971
2014-01-15 10:02:31 -08:00
epriestley
42d9fa34e2 Use RemarkupControl in Differential inline comment UI
Summary: Fixes T4317. Update the "inline comment" control to a RemarkupControl. This could maybe use some padding/spacing/design touches eventually but seems OK for the moment.

Test Plan: {F101825}

Reviewers: chad, btrahan

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T4317

Differential Revision: https://secure.phabricator.com/D7969
2014-01-14 15:15:47 -08:00
Chad Little
31a2bebf63 Move PhabricatorTagView to PHUITagView
Summary: For consistency and great justice.

Test Plan: tested audit, uiexamples, action headers

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7967
2014-01-14 14:09:52 -08:00
epriestley
a716fe99f3 Perform search indexing in the worker queue and respect bin/search index --background
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.

Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3857

Differential Revision: https://secure.phabricator.com/D7966
2014-01-14 13:22:56 -08:00
Chad Little
51d8570b34 Clean up diff view page
Summary: Cleans up some older layouts to new stuffs.

Test Plan: Test with and without a diff ID.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7949
2014-01-13 12:17:37 -08:00
Chad Little
b74c7a3d37 Simplify PHUIObjectBoxViews handling of Save and Error states
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.

Test Plan: Test out the forms, see errors without the text.

Reviewers: epriestley, btrahan

CC: Korvin, epriestley, aran, hach-que

Differential Revision: https://secure.phabricator.com/D7924
2014-01-10 09:17:37 -08:00
epriestley
68de639470 Fix two edge case issues with Differential mail tags
Summary:
Via Asana. The tags on Differential mail are wrong in two cases:

  - Transactions which submit inline comments but no comment text are not labeled as "comments", but should be.
  - Non-close, non-comment transactions are not labeled at all, but should be labeled "other".

Test Plan: Submitted a no-comments, inlines-only transaction and got a message with proper `X-Phabricator-Mail-Tags` header.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7912
2014-01-09 10:56:27 -08:00
Chad Little
3c5756adf9 Clean up AphrontError boxes, Diffusion Headers
Summary: Two basic changes here, first we fixed up the Diffusion headers to roll out more PHUIObjectBoxes. Second we added some specific styles for when Errors are inside an ObjectBox at the first position.

Test Plan: Tested a number of different layouts for browsing respositories as well as wherever I could find cases with PHUIObjectBox Form Errors (see images attached). Still some minor tightening due after this diff, but didnt want to overload it.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7914
2014-01-09 08:51:57 -08:00
epriestley
31b6f69ff7 Allow CelerityResourceResponse to hold resources from multiple maps
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.

Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.

Test Plan:
  - Reloaded pages.
  - Browsed around Differential.

Reviewers: btrahan, hach-que

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4222

Differential Revision: https://secure.phabricator.com/D7876
2014-01-02 11:59:35 -08:00
Aviv Eyal
2de96c6f08 Semi-fix git cloning for non-bare repos for landing
Summary:
If the repo isn't bare, than we need copy it's remote instead of using it.

This will probably not work if an SSH key is provided to phabricator, and in any case you must delete
all workspaces that were already created.

This will make landing those repos slower; I plan to just delete and re-clone all repos on my instance.

It will probably be simpler to just make a bare-repo a requirement of all the git-landing work.

Test Plan: landed from hosted and un-hosted repos, checked git-remote url in each newly cloned workspace.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7592
2013-12-30 15:22:32 -08:00
epriestley
591df78361 Bind patches, file content and raw diffs bind policies to their originating objects
Summary:
Fixes T4270. When you download raw file content, diffs, and patches we currently give them default (all users) visibility.

Instead, bind them to the repository or revision in question.

(This code could use a bit of cleanup at some point.)

Test Plan: Hit the patch and content download links in Diffusion and the patch download link in Differential, got restricted files with accurate policy bindings.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4270

Differential Revision: https://secure.phabricator.com/D7849
2013-12-30 11:27:02 -08:00
epriestley
aad6b57c36 Add bin/harbormaster to make builds easier to debug
Summary:
Ref T1049. Adds `bin/harbormaster` and `bin/harbormaster build` for applying plans from the console. Since this gets `--trace`, it's much easier to debug what's going on.

This doesn't work properly with some of the Drydock steps yet, I need to look at those. I think `setRunAllTasksInProcess` probably obsoletes some of the mechanisms. It might also not work with "Wait for Builds" but I didn't check.

Test Plan: Used `bin/harbormaster` to run a bunch of builds. Ran builds from web UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7825
2013-12-26 10:40:52 -08:00
epriestley
a5dc9067af Provide convenience method addTextCrumb() to PhabricatorCrumbsView
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.

Test Plan:
  - This was mostly automated, then I cleaned up a few unusual sites manually.
  - Bunch of grep / randomly clicking around.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: hach-que, aran

Differential Revision: https://secure.phabricator.com/D7787
2013-12-18 17:47:34 -08:00
epriestley
5863f792a6 Remove many redundant implementations of canLoadNamedObject()
Summary:
These just got copy/pasted like crazy, the base class has the correct default implementation.

(I'm adding "H" for Herald Rules, which is why I was in this code.)

I also documented the existing prefixes at [[ Object Name Prefixes ]].

Test Plan: Verified base implementation. Typed some object names into the jump nav.

Reviewers: btrahan

Reviewed By: btrahan

CC: hach-que, aran

Differential Revision: https://secure.phabricator.com/D7785
2013-12-18 12:00:01 -08:00
Aviv Eyal
de969ab540 plug octocat icon to landing button
Test Plan: look at shiny button

Reviewers: epriestley, chad, #blessed_reviewers

Reviewed By: chad

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7749
2013-12-09 13:29:34 -08:00
epriestley
ff4e965c90 Don't try to download diffs-of-diffs
Summary:
Ref T1715. When the user clicks "Download Raw Diff" in Differential, we try to build a diff of exactly what they're seeing. However:

  - This doesn't work if any of the changes have multiple hunks, and fixing it seems hard.
  - I suspect this diff is never actually useful anyway? And probably kind of confusing in the best case. You can't really apply it to anyhting, since you'd have to apply another diff first.

Instead, just build the right-side diff, which should align well with user expectation and doesn't suffer from the multi-hunk bug.

Some day, we could maybe add some of the fancy options in T1715.

See: <https://github.com/facebook/phabricator/issues/461>

Test Plan: Downloaded a multi-hunk diff, got the original back and applied it cleanly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1715

Differential Revision: https://secure.phabricator.com/D7694
2013-12-04 14:39:07 -08:00
epriestley
b435c03eca Add a config flag to treat "Accepted" revisions as "Closed"
Summary: See D7653. This is exclusively for Asana, who uses Differential for a post-commit, Audit-like workflow but has a small set of requirements for it to be a good fit (just this) and a large set of requirements for Diffusion/Audit to be a good fit.

Test Plan: Set the flag, verified "Accepted" revisions are no longer on the dashboard.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7654
2013-11-25 17:40:01 -08:00
epriestley
1da691113a Normalize the definition of "closed" revision statuses
Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.

The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.

To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.

Also delete the very old COMMITTED status, which has been obsolete for over a year.

Test Plan: Browsed around most/all of the affected interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7653
2013-11-25 17:39:24 -08:00
epriestley
8b19465a9d Render linked JIRA issues with a Doorkeeper tag
Summary: Fixes T3687. Instead of rendering "JIRA Issues" in Differential using plain links, render them using Doorkeeper tags so they get the nice "enhance with object name" effect.

Test Plan: {F84886}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D7648
2013-11-25 14:54:35 -08:00
Kelsey Fix
a6b16bb894 Fixing usage message for "hg diff"
Summary: Phabricator doesn't accept raw hg diff, fixing usage message to
specify using git extended diff.

See: <https://github.com/facebook/phabricator/pull/444>

Reviewed by: epriestley
2013-11-19 17:51:47 -08:00
Joel Beales
9efcbc4ee9 Speed up loading of diffs with a lot of unit test failures
Summary:
We've been having trouble with viewing diffs timing out when there's a lot of unit test failures. It was caused by formatting userdata for every single failure. The expensive part of this was actually creating the engine for every result, so moved the construction outside of the loop.

Diffs that timed out (2 min) loading before load in around 6 seconds now.

Test Plan: Loaded diffs that used to time out. Verified that details still looked right when Show Full Unit Test Results Is Clicked.

Reviewers: epriestley, keegancsmith, lifeihuang, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, andrewjcg

Differential Revision: https://secure.phabricator.com/D7581
2013-11-19 15:19:15 -08:00
epriestley
c207964036 Never raise policy exceptions for the omnipotent viewer
Summary:
Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.

Fix this in two ways:

  # Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
  # In this case, load the revision for an omnipotent viewer.

This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.

Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4109

Differential Revision: https://secure.phabricator.com/D7603
2013-11-19 14:10:38 -08:00
Aviv Eyal
dcf909ba56 Land to GitHub + support stuff
Summary:
A usable, Land to GitHub flow.

Still to do:
- Refactor all git/hg stratagies to a sane structure.
- Make the dialogs Workflow + explain why it's disabled.
- Show button and request Link Account if GH is enabled, but user is not linked.
- After refreshing token, user ends up in the settings stage.

Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.

Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.

Test Plan:
Went through these flows:
- load revision in hosted, github-backed, non-github backed repos to see button as needed.
- hit land with weak token - sent to refresh it with the extra scope.
- Land to repo I'm not allowed - got proper error message.
- Successfully landed; Failed to apply patch.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D7555
2013-11-13 17:25:24 -08:00
epriestley
c818e6e159 Remove differential.anonymous-access
Summary:
Fixes T3034. This is obsoleted by modern policies.

This was written by a Facebook intern and is rarely used -- the Hive install might be the only use in the wild. It has never really worked correctly.

Test Plan: `grep`; browsed Differential.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3034

Differential Revision: https://secure.phabricator.com/D7568
2013-11-11 16:05:19 -08:00
Bob Trahan
819bd7f03b Add arcanist project phid to differential.query
Summary: Fixes T3535. Also, flip flop on that spacing thing and make the spaces purdy

Test Plan: got an arcanist projected phid in the json dict

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3535

Differential Revision: https://secure.phabricator.com/D7565
2013-11-11 15:01:21 -08:00
Jakub Vrana
a29b5b070f Replace some hsprintf() by phutil_tag()
Test Plan: Looked at a diff with inline comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7549
2013-11-11 09:23:23 -08:00
James Rhodes
7ffea0463e Use herald to trigger builds of revisions and commits.
Summary:
Depends on D7500.

This seemed like a pretty good idea once I thought of it.  Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions.  This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).

Test Plan: Ran the usual daemons + the Harbormaster daemon.  Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up.  Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, hwinkel

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7501
2013-11-08 16:58:39 -08:00
Asher Baker
d700e7f22d Add support for landing to hosted Mercurial repos.
Summary: I've kept this as close as possible to the Git version for ease of review and later refactoring of them both together. At minimum, the functions to get the working dir should probably be cleaned up one day.

Test Plan: Landed a revision.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7534
2013-11-08 11:39:32 -08:00
epriestley
4f0f95f7b5 Assign PHIDs to all diffs
Summary:
Ref T1049. Ref T2222. `DifferentialDiff` does not currently have a PHID, but we need it for Harbormaster and ApplicationTransactions. See some discussion in D7501.

(I split the SQL into two sections so we can't fail in the middle. At some point, I'd like to do a pass on the migration stuff and get this happening automatically, and also simplify the PatchList.)

Test Plan:
  - Ran `bin/storage upgrade`.
  - Checked for valid PHIDs in the database.
  - Used `phid.query` to look up a diff by PHID.
  - Created a new diff and verified it got a PHID.

Reviewers: btrahan, hach-que

Reviewed By: btrahan

CC: aran, vrana

Maniphest Tasks: T2222, T1049

Differential Revision: https://secure.phabricator.com/D7513
2013-11-06 13:59:06 -08:00
Aviv Eyal
5c0edc9351 Land Revision button for hosted git repos
Summary:
ref T182.

Simple approach of clone, patch, push. While waiting for drydock, implement a hackish mutex
setup for the workspace, which should work ok as long as there's only one committer who is
carefull about theses things.

Less obvious note: This is taking the both author and commiter's 'primary email' for the commit -
which might rub some people wrong.

Test Plan:
With a hosted repo, created some diffs and landed them.
Also clicked button for some error cases, got the right error message.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: hach-que, Korvin, epriestley, aran

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D7486
2013-11-05 13:00:13 -08:00
Jakub Vrana
00fb8c22e4 Decentralize phtize
Summary: The idea is to have all `phtize` definitions in applications to allow their separation.

Test Plan: Clicked View Options after mangling the translation.

Reviewers: epriestley

Reviewed By: epriestley

CC: btrahan, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7345
2013-11-04 11:38:28 -08:00
Chad Little
1c31ea3a60 Add header icons to PHUIPropertyListView
Summary: Adds summary (description) and test plan icons to make these area's more unique and differentiated over general sections.

Test Plan: Test a diff, a commit, a task

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7493
2013-11-04 11:07:51 -08:00
epriestley
2f7057ad99 Preserve linebreaks again in Summary / Test Plan for revisions
Summary:
I pulled these into the property list recently, which made them more consistent, but that dropped "preserve linebreaks". Since these usually come from the CLI, render with linebreaks preserved.

@csilvers, you'll need to `bin/cache purge --purge-remarkup` after this if you want to fix existing revisions.

Test Plan: Made a revision with some poetry, saw poetry preserved.

Reviewers: btrahan, csilvers

Reviewed By: btrahan

CC: csilvers, aran

Differential Revision: https://secure.phabricator.com/D7464
2013-10-30 17:20:06 -07:00
epriestley
29c7de621e Split exceptionally huge revision titles into the summary
Summary: Fixes T4034.

Test Plan: See screenshot.

Reviewers: spicyj, btrahan

Reviewed By: spicyj

CC: aran

Maniphest Tasks: T4034

Differential Revision: https://secure.phabricator.com/D7443
2013-10-29 21:19:51 -07:00
Chad Little
cdcfbb69a0 CSS Tweaks to reviewers status list
Summary: Updates the review status list to align better inside property lists. Alsu uses the default colors a bit more. This removes an overflow hidden on the value side, but that shouldnt cause any issues, given it has plenty of space.

Test Plan: tested differential and audit, highlighted and not.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7441
2013-10-29 20:26:41 -07:00
Bob Trahan
da84546058 Add filter by object ability to flag query
Summary: See title. Fixes T1809.

Test Plan:
verified each type that has flaggable interface still can be flagged

verified that new custom query filter works

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T1809

Differential Revision: https://secure.phabricator.com/D7392
2013-10-25 12:52:00 -07:00
epriestley
9e87172166 Make remarkup rules runtime-pluggable in a reasonable way
Summary:
Gets rid of some old Differential-specific nonsense and replaces it with general runtime-pluggable Remarkup rules.

Facebook: This removes two options which may be in use. Have any classes being added via config here just subclass the new abstract bases instead. This should take 5 seconds to fix. You can adjust order by overriding `getPriority()` on the rules, if necessary.

Test Plan: See comments.

Reviewers: btrahan

Reviewed By: btrahan

CC: FacebookPOC, andrewjcg, aran

Differential Revision: https://secure.phabricator.com/D7393
2013-10-24 17:26:07 -07:00
epriestley
2a5c987c71 Lock policy queries to their applications
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.

This has several parts:

  - For PolicyAware queries, provide an application class name method.
  - If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
  - For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.

Test Plan:
  - Added a unit test to verify I got all the class names right.
  - Browsed around, logged in/out as a normal user with public policies on and off.
  - Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7367
2013-10-21 17:20:27 -07:00
epriestley
32dd8af9e5 Reduce surface area of DifferentialComment API
Summary:
Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions.

Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc.

Test Plan: Browsed several revisions.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3415, T2222

Differential Revision: https://secure.phabricator.com/D7375
2013-10-21 17:01:27 -07:00
epriestley
8994a81b35 Make event-triggered actions more aware of application access
Summary:
Fixes T3675.

  - Maniphest had a couple of old non-event listeners; move them to events.
  - Make most of the similar listeners a little more similar.
  - Add checks for access to the application.

Test Plan:
  - Viewed profile, project, task, revision.
  - Clicked all the actions.
  - Blocked access to various applications and verified the actions vanished.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3675

Differential Revision: https://secure.phabricator.com/D7365
2013-10-21 17:00:50 -07:00
epriestley
d66972c9f2 Tie application event listeners to the applications they listen for
Summary:
Ref T3675. Some of these listeners shouldn't do their thing if the viewer doesn't have access to an application (for example, users without access to Differential should not be able to "Edit Tasks"). Set the stage for that:

  - Introduce `PhabricatorEventListener`, which has an application.
  - Populate this for event listeners installed by applications.
  - Rename the "PeopleMenu" listeners to "ActionMenu" listeners, which better describes their modern behavior.

This doesn't actually change any behaviors.

Test Plan: Viewed Maniphest, Differntial, People.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3675

Differential Revision: https://secure.phabricator.com/D7364
2013-10-21 17:00:21 -07:00
epriestley
c69beb7988 Stop writes to the old Relationship table
Summary: Ref T1279. The new stuff seems stable, so stop writes to the old tables.

Test Plan:
  - Added and removed reviewers.
  - Grepped for `::RELATIONSHIP_TABLE` to verify we really have no more reads.
  - Grepped for `::RELATION_REVIEWER`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7360
2013-10-21 16:59:22 -07:00
epriestley
00bf47f973 Fix "Manage herald rules" link by removing it
Summary: Fixes T4001. I broke this some time ago and no one has complained. I don't think it gets much use, and we haven't added it for the newer apps. Just get rid of it rather than adapt the URIs for ApplicationSearch.

Test Plan: Unit tests, sent myself some email.

Reviewers: zeeg, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4001

Differential Revision: https://secure.phabricator.com/D7355
2013-10-21 16:58:56 -07:00
epriestley
31c474a7ff Duct-tape the "z" haunted comment panel mode back together
Summary: Fixes T3898. This feature needs generalization at some point, but just unbreak it for now since a surprising number of users like it.

Test Plan: Pressed "z".

Reviewers: chad, btrahan

Reviewed By: chad

CC: chad, aran, spicyj

Maniphest Tasks: T3898

Differential Revision: https://secure.phabricator.com/D7366
2013-10-19 16:43:33 -07:00
epriestley
f010730e49 Migrate all Differential inline comments to ApplicationTransactions
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.

The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.

Two risks here:

  - I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
  - This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.

I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.

Test Plan:
  - Before migrating, then after migrating:
    - Made a bunch of inlines (drafts, submitted).
    - Edited and deleted inlines.
    - Verified inlines showed up in preview.
    - Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
    - Verified that inlines ARE indexed when they're not drafts.
    - Verified that drafts inlines make revisions appear as "with draft" in the revision list.
  - Made left, right, and draft inlines.
  - Migrated (`bin/storage upgrade`).
  - Verified that my inlines from before the migration still showed up.
  - (Repeated all the stuff above.)
  - Manually inspected the inline comment table.

Reviewers: btrahan

Reviewed By: btrahan

CC: FacebookPOC, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D7139
2013-10-19 05:03:25 -07:00
epriestley
32dca4b553 Fix lightbox downloads for embeded images and a warning
Summary:
I refactored this recently and accidentally dropped the download URI.

Also fix a warning with, e.g., files named `README`.

Test Plan: Clicked a thumb, clicked "Download", got a file.

Reviewers: chad, btrahan, dctrwatson

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7341
2013-10-17 11:21:01 -07:00
Jakub Vrana
a9a3bdf3cc Delete unintentional phlog()
Leaked in D7329.
2013-10-16 13:59:23 -07:00
Jakub Vrana
29391a658e Disallow <! in <script>
Summary:
HTML5 has this crazy script escaping states:

- Script data escaped dash dash state
- Script data double escaped state

https://communities.coverity.com/blogs/security/2012/11/16/did-i-do-that-html-5-js-escapers-3

Perhaps `<!` is too aggressive but I didn't spend much time searching for a more fine grained expression.

Test Plan: Searched for `renderInlineScript()`.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7329
2013-10-16 09:28:37 -07:00
Chad Little
451dfb9afb Differential Changeset header icons
Summary: Adds filetype icons, applying to differential file headers. The main issue is with all the lightening, I wanted something to still anchor 'new file' on the page and adding a sharp icons does that pretty well for me. Feedback is cool too.

Test Plan: Add some new icons, test in previous commits.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7320
2013-10-15 07:34:48 -07:00
epriestley
073cb0e78c Make PhabricatorPolicyInterface require a getPHID() method
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.

Some policy objects don't have real PHIDs:

  PhabricatorTokenGiven
  PhabricatorSavedQuery
  PhabricatorNamedQuery
  PhrequentUserTime
  PhabricatorFlag
  PhabricatorDaemonLog
  PhabricatorConduitMethodCallLog
  ConduitAPIMethod
  PhabricatorChatLogEvent
  PhabricatorChatLogChannel

Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.

Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.

Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7306
2013-10-14 14:35:47 -07:00
Chad Little
97c690fc0f PHUIPropertyListView
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.

Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7283
2013-10-11 07:53:56 -07:00
epriestley
f4582dc49d Allow "Default View" policies to be set to Public
Summary: Ref T603. Currently, we hard-code defense against setting policies to "Public" in several places, and special case only the CAN_VIEW policy. In fact, other policies (like Default View) should also be able to be set to public. Instead of hard-coding this, move it to the capability definitions.

Test Plan: Set default view policy in Maniphest to "Public", created a task, verified default policy.

Reviewers: btrahan, asherkin

Reviewed By: asherkin

CC: asherkin, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7276
2013-10-09 15:06:18 -07:00
epriestley
436a403357 Add a "default view" policy to Differential
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.

I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.

Test Plan: Created new revisions from the CLI and conduit.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7269
2013-10-09 13:58:00 -07:00
epriestley
b1b1ff83f2 Allow applications to define new policy capabilities
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.

Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.

That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.

Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.

Also provide more context, and more useful exception messages.

Test Plan:
  - See screenshots.
  - Also triggered a policy exception and verified it was dramatically more useful than it used to be.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7260
2013-10-07 13:28:58 -07:00
epriestley
0598600476 Always pass handles to tokenizers, not <phid -> name> maps
Summary: Ref T1279. Prerequisite for adding icons or other type information to tokenizers, since we don't currently have enough information to prefill them when rendering things from the server side. By passing handles in, the tokenizer can extract type information.

Test Plan:
- Searched by user in Audit.
- Sent Conpherence from profile page.
- Tried to send an empty conpherence.
- Searched Countdown by user.
- Edited CCs in Differential.
- Edited reviewers in Differential.
- Edited a commit's projects.
- Searched lint by owner.
- Searched feed by owner/project.
- Searched files by owner.
- Searched Herald by owner.
- Searched Legalpad by owner.
- Searched Macro by owner.
- Filtered Maniphest reports by project.
- Edited CCs in Maniphest.
- Searched Owners by owner.
- Edited an Owners package.
- Searched Paste by owner.
- Searched activity logs by owner.
- Searched for mocks by owner.
- Edited a mock's CCs.
- Searched Ponder by owner.
- Searched projects by owner.
- Edited a Releeph project's pushers.
- Searched Releeph by requestor.
- Edited "Uses Symbols" for an Arcanist project.
- Edited all tokenizers in main search.
- Searched Slowvote by user.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7248
2013-10-07 12:51:24 -07:00
epriestley
2abbd51868 Don't raise a policy exception if a user can't see the parent revision of a new diff
Summary: Ref T603. We currently bomb out here, but should just continue forward. I'm fairly certain we don't even use this for anything anymore (it has been replaced by "depends on") but need to check that.

Test Plan: Created a new revision with `arc diff`.

Reviewers: ljalonen, btrahan, #blessed_reviewers, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7255
2013-10-07 12:51:04 -07:00
epriestley
c6add6ae73 Make "reject" and "blocking reviewer" block acceptance in Differential
Summary:
Ref T1279. This is a logical change.

  - "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
  - "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".

Practically, the primary net effect of this is just to make blocking reviews actually block.

This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.

Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, wisutsak.jaisue.7

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7245
2013-10-06 17:09:56 -07:00
epriestley
8aa8ef49da Provide an "Add blocking reviewer..." Herald action
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.

Test Plan: Added Herald rules and updated revisions; see screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7244
2013-10-06 17:09:24 -07:00
epriestley
929ad86b57 Allow accepting accepted revisions, and rejecting rejected revisions
Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.

This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.

Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7243
2013-10-06 17:09:02 -07:00
epriestley
d518f3c9de When a user accepts a revision, accept for all projects the user has authority over
Summary: Ref T1279. If you accept a revision, also accept on behalf of all the projects you have authority to accept for.

Test Plan:
  - Accepted a revision which I was a reviewer on, saw my own status and an authority project's status change to "Accepted".
  - Accepted a revision which I was not a reviewer on, saw my own status be added (as "Accepted") and the project's status update.

Also, see screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, wisutsak.jaisue.7

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7242
2013-10-06 17:08:30 -07:00
epriestley
3d3d3b6d80 Move determination of reviewer authority into DifferentialRevisionQuery
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:

  - It puts queries very close to the display layer.
  - We have to query for each revision if we want to figure out authority for several.
  - We need to figure it out in several places, so we'll end up with copies of this logic.
  - The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
  - We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.

Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.

Test Plan:
  - Looked at some revisions, verified the correct lines were highlighted.
    - Looked at a revision I created and verified that projects I was a member of were not highlighted.
      - With self-accept enabled, these //are// highlighted.
    - Looked at a revision I did not create and verified that projects I was a member of were highlighted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7241
2013-10-06 17:08:14 -07:00
epriestley
c80a4f51c1 Highlight reviews the viewer is responsible for in Differential
Summary: Ref T1279. No logical changes, but cosmetically highlight stuff you have authority for, like we do in Diffusion.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7237
2013-10-05 14:10:52 -07:00
epriestley
4c0ec01ce5 Allow Herald rules to add reviewers
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.

I will probably write some lengthy treatise on how you shouldn't use this rule later.

Implementation is straightforward because Differential previously supported this rule.

This rule can also be used to add project reviewers.

Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7235
2013-10-05 14:10:51 -07:00
epriestley
2d733f88a1 Split users apart from projects/packages in reviewer and audit UIs
Summary: Ref T1279. Show separate sections for "Reviewers" and "Project Reviewers" (Differential) and for "Auditors" and "Package/Project Auditors" (Diffusion/Audit).

Test Plan:
  - Looked at a commit. Saw separation.
  - Looked at a revision. Saw separation.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7233
2013-10-05 14:10:49 -07:00
epriestley
9434df9d7c Accommodate project reviewers in Differential search
Summary:
Ref T1279. Two changes to the search/query for Differential:

  - "Reviewers" now accepts users and projects.
  - "Responsible Users" now includes revisions where a project you are a member of is a reviewer.

Test Plan:
  - Searched for project reviewers.
  - Verified that the dashboard now shows reviews which I'm only part of via project membership.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7231
2013-10-05 14:10:47 -07:00
epriestley
cf4eb3109e Allow projects to review revisions
Summary:
Ref T1279. No actual logical changes, but:

  - You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
  - You can now add projects as reviewers from the revision detail typeahead.
  - You can now add projects as reviewers from the CLI (`#yoloswag`).
  - Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).

I'll separate projects from users in the "Reviewers" tables in the next revision.

Test Plan:
  - Added projects as reviewers using the web UI and CLI.
  - Used `arc amend --show --revision Dnnn` to generate commit messages.
  - Viewed revision with project reviewers in web UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7230
2013-10-05 14:10:46 -07:00
epriestley
370c7635a7 Track "accepted" and "commented" in per-reviewer status
Summary: Ref T1279. Updates status to 'accepted' or 'commented' when the user takes those actions.

Test Plan:
  - Commented on a revision, got a comment icon.
  - Accepted a revision, got an accept icon.
  - Commented again, icon stayed as "accept".
  - Faked the "old diff" states.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7229
2013-10-05 14:10:45 -07:00
epriestley
4d8707df13 Use status list UI to show reviewers in Differential
Summary:
Ref T1279. No logical changes, just updates the reviewer display style.

We currently keep track of only "requested changes".

Test Plan: See screenshot.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7228
2013-10-05 14:10:44 -07:00
epriestley
65ddefad8b Migrate all Differential reviewer data to edges
Summary:
Ref T1279. @champo did a lot of this work already; we've been doing double writes for a long time.

Add "double reads" (reading the edge table as both the "relationship" table and as the "reviewer status" table), and migrate all the data.

I'm not bothering to try to recover old reviewer status (e.g., we could infer from transactions who accepted old revisions) because it wold be very complicated and doesn't seem too valuable.

Test Plan:
  - Without doing the migration, used Differential. Verified that reads and writes worked. Most of the data was there anyway since we've been double-writing.
  - Performed the migration. Verified that everything was still unchanged.
  - Dropped the edge table, verified all reviweer data vanished.
  - Migrated again, verified the reviewer stuff was restored.
  - Did various cc/reviewer/subscriber queries, got consistent results.

Reviewers: btrahan

Reviewed By: btrahan

CC: champo, aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7227
2013-10-05 13:54:02 -07:00
Neal Poole
1a5de83ad1 Add support for bookmarks in Phabricator emails.
Summary: Right now emails don't include bookmark info (wasn't added in D2897). Lets include it so it's consistent with the web UI.

Test Plan: Inspected code, made sure it matched web UI code. Verified that web UI with these changes was consistent with rendering before refactoring.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7215
2013-10-04 11:19:38 -07:00
Neal Poole
1edb875978 Adding support for 'adds' and 'removes' in diff content.
Summary:
Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'.
- There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant.
- The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally.

Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3829

Differential Revision: https://secure.phabricator.com/D7214
2013-10-04 06:37:39 -07:00
David Cramer
eb548f5af7 Add differential.getrawdiff to Conduit
Test Plan: Confirm the API returns a single flat result with a unified git diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran, charles

Differential Revision: https://secure.phabricator.com/D7199
2013-10-02 17:03:53 -07:00
David Cramer
3c34cdce5a Abstract raw diff rendering into DifferentialRawDiffRenderer
Test Plan:
Enable inline patches:

```
bin/config set metamta.differential.patch-format 'unified'
bin/config set metamta.differential.inline-patches 100000000
```

Create a new diff and confirm it renders correctly via email.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7198
2013-10-02 16:28:57 -07:00
epriestley
ea5bc2efac Remove some dead code
Summary: I removed the only callsite in D7179, but forgot to remove this code.

Test Plan: Grepped for callsites.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7194
2013-10-02 13:11:35 -07:00
epriestley
fc57995330 Fix a typo in differential.querydiffs
Summary:
  - "revision" is misspelled.
  - Remove an unused variable.

Test Plan: Used API console to call method.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7184
2013-09-30 17:49:23 -07:00
epriestley
b21f197a98 Fix Diffusion change view header
Summary: See D7162. This was like 99% my fault. Just provide a header; the new ones look pretty reasonable.

Test Plan: Viewed Diffusion change view, no exception.

Reviewers: vrana, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7183
2013-09-30 17:49:15 -07:00
epriestley
dd206a5b69 Viewerize ArcBundle file loading callbacks
Summary: Ref T603. Clean these up and move them to a single place.

Test Plan:
  - Downloaded a raw diff.
  - Enabled "attach diffs", created a revision, got an email with a diff.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7179
2013-09-30 12:21:33 -07:00
epriestley
13dae05193 Make most file reads policy-aware
Summary: Ref T603. Swaps out most `PhabricatorFile` loads for `PhabricatorFileQuery`.

Test Plan:
  - Viewed Differential changesets.
  - Used `file.info`.
  - Used `file.download`.
  - Viewed a file.
  - Deleted a file.
  - Used `/Fnnnn` to access a file.
  - Uploaded an image, verified a thumbnail generated.
  - Created and edited a macro.
  - Added a meme.
  - Did old-school attach-a-file-to-a-task.
  - Viewed a paste.
  - Viewed a mock.
  - Embedded a mock.
  - Profiled a page.
  - Parsed a commit with image files linked to a revision with image files.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7178
2013-09-30 09:38:13 -07:00
epriestley
b592630d72 Provide more structure to PHUIObjectBoxView
Summary:
Three changes here.

  - Add `setActionList()`, and use that to set the action list.
  - Add `setPropertyList()`, and use that to set the property list.

These will let us add some apropriate CSS so we can fix the border issue, and get rid of a bunch of goofy `.x + .y` selectors.

  - Replace `addContent()` with `appendChild()`.

This is just a consistency thing; `AphrontView` already provides `appendChild()`, and `addContent()` did the same thing.

Test Plan:
  - Viewed "All Config".
  - Viewed a countdown.
  - Viewed a revision (add comment, change list, table of contents, comment, local commits, open revisions affecting these files, update history).
  - Viewed Diffusion (browse, change, history, repository, lint).
  - Viewed Drydock (resource, lease).
  - Viewed Files.
  - Viewed Herald.
  - Viewed Legalpad.
  - Viewed macro (edit, edit audio, view).
  - Viewed Maniphest.
  - Viewed Applications.
  - Viewed Paste.
  - Viewed People.
  - Viewed Phulux.
  - Viewed Pholio.
  - Viewed Phame (blog, post).
  - Viewed Phortune (account, product).
  - Viewed Ponder (questions, answers, comments).
  - Viewed Releeph.
  - Viewed Projects.
  - Viewed Slowvote.

NOTE: Images in Files aren't on a black background anymore -- I assume that's on purpose?

NOTE: Some jankiness in Phortune, I'll clean that up when I get back to it. Not related to this diff.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7174
2013-09-30 09:36:04 -07:00
epriestley
6a74ad724b Fix an issue with UUID query construction
Summary: This is SVN-only and I missed it in my test plan.

Test Plan: `arc diff` in SVN repository with no `.arcconfig`.

Auditors: btrahan
2013-09-30 03:55:07 -07:00
Chad Little
fde23fe77c ObjectBoxView for Open Revisions
Summary: Missed this case in my sandbox

Test Plan: Reload a test diff

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7168
2013-09-28 16:04:17 -07:00
Chad Little
94d0704fdb Add objectheaders to new View
Summary: This adds the 'PHUIObjectBox' to nearly every place that should get it. I need to comb through Diffusion a little more. I've left Differential mostly alone, but may decide to do it anyways this weekend. I'm sure I missed something else, but these are easy enough to update.

Test Plan: tested each new layout.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7162
2013-09-28 15:55:38 -07:00
epriestley
5799e8e2de Provide better strings in policy errors and exceptions
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.

  - Provide as much detail as possible.
  - Fix all the strings for i18n.
  - Explain special rules to the user.
  - Allow indirect policy filters to raise policy exceptions instead of 404s.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7151
2013-09-27 08:43:50 -07:00
epriestley
2e5ac128b3 Explain policy exception rules to users
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".

This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.

Test Plan: See screenshot.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7150
2013-09-27 08:43:41 -07:00
epriestley
e0f99484ac Make Differential views capability-sensitive
Summary:
Ref T603. Make Differential behaviors for logged-out and underprivleged users more similar to other apps.

I'm going to drop this "anonymous access" thing at some point, but `reviews.fb.net` actually looks like it's running semi-modern code, so leave it alive until we have a more compelling replacement in the upstream.

Test Plan: As a logged out user, browsed Differential and clicked things and such.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7148
2013-09-26 18:45:04 -07:00
epriestley
874a9b7fe3 When creating or updating a revision, infer the repository from the diff
Summary:
Ref T603. When a diff is attached to a revision, try to guess the repository if possible. In cases where we succeed, this automatically gives us intuitive policy behavior (i.e., you can see a revision if you can see the repository the change is against).

I pulled this into a funky little "Lookup" class for two reasons:

  - It's used in two places;
  - I anticipate that we might need to add some sort of `explainWhy()` method if users find the heuristics confusing.

Test Plan: Created and updated revisions, saw them pick up the correct repository association. Ran Herald dry run against associable and nonassociable revisions, saw correct values populate.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7147
2013-09-26 15:28:42 -07:00
epriestley
b435c0297e Allow revisions to be queried by repository
Summary: This isn't too useful most of the time since we don't automatically populate this data yet, but works fine.

Test Plan: See screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7144
2013-09-26 14:17:26 -07:00
epriestley
3d354d205f Allow editPolicy, viewPolicy, and repositoryPHID to be edited from the web UI in Differential
Summary: Ref T603. I think T2222 is fraught with peril so I'm not going to try to sequence it ahead of T603 for Differential. Provide access to policy controls in Differential's edit view.

Test Plan: See screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7142
2013-09-26 14:17:11 -07:00
Chad Little
7421a42ba5 Fix spacing on Diff headers
Summary: Adds some padding to the right

Test Plan: Looked at a diff

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7143
2013-09-26 14:08:42 -07:00
epriestley
5677cd23bd Add storage and classes for CustomField in Differential
Summary: Ref T3886. Adds the storage, indexes, and storage classes for modernizing Differential custom fields.

Test Plan: Ran `storage upgrade`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D7138
2013-09-26 12:37:28 -07:00
epriestley
9b3d7b0dba Make most Differential reads policy-aware
Summary: Ref T603. Makes the majority of reads policy aware (and pretty much all the important ones).

Test Plan:
  - Created a comment with `differential.createcomment`.
  - Created a new revision with `arc diff` in order to exercise `differential.creatediff`.
  - Created an inline comment with `differential.createinline`.
  - Added a comment to a revision.
  - Edited an inline comment.
  - Edited a revision.
  - Wrote "Depends on ..." in a summary, saved, verified link was created.
  - Browsed a file in Diffusion.
  - Got past the code I changed in the Releeph request thing.
  - Edited a Releeph request.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7136
2013-09-26 12:37:19 -07:00
epriestley
80378eb5f6 Show policy information in Differential header
Summary: Ref T603. Moves policy information from a custom field to the header for revisions.

Test Plan: Looked at a revision.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7135
2013-09-26 12:37:05 -07:00
epriestley
d61c931c7b Use Differential policy columns to drive policies
Summary:
Ref T603. Read policies out of policy columns.

When a revision is associated with a repository (which is currently never), require view access on the repository to see the revision (or, require the viewer to be the owner). This is a blanket "do the right thing" rule which should make Differential's default policies align with user expectations.

Future diffs will populate the `repositoryPHID` when a revision is created.

Test Plan: Tooled around Differential. None of this stuff does anything yet, so nothing very exciting happened.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7134
2013-09-26 12:36:45 -07:00
epriestley
c458517cb4 Add viewPolicy, editPolicy, repositoryPHID columns to DifferentialRevision
Summary: Ref T603. Paves the way for policy controls.

Test Plan: Ran storage upgrade, bumbled around in Differential.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7133
2013-09-26 12:36:30 -07:00
epriestley
c467cc464f Make most repository reads policy-aware
Summary: Ref T603. This swaps almost all queries against the repository table over to be policy aware.

Test Plan:
  - Made an audit comment on a commit.
  - Ran `save_lint.php`.
  - Looked up a commit with `diffusion.getcommits`.
  - Looked up lint messages with `diffusion.getlintmessages`.
  - Clicked an external/submodule in Diffusion.
  - Viewed main lint and repository lint in Diffusion.
  - Completed and validated Owners paths in Owners.
  - Executed dry runs via Herald.
  - Queried for package owners with `owners.query`.
  - Viewed Owners package.
  - Edited Owners package.
  - Viewed Owners package list.
  - Executed `repository.query`.
  - Viewed "Repository" tool repository list.
  - Edited Arcanist project.
  - Hit "Delete" on repository (this just tells you to use the CLI).
  - Created a repository.
  - Edited a repository.
  - Ran `bin/repository list`.
  - Ran `bin/search index rGTESTff45d13dffcfb3ea85b03aac8cc36251cacdf01c`
  - Pushed and parsed a commit.
  - Skipped all the Drydock stuff, as it it's hard to test and isn't normally reachable.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7132
2013-09-25 16:54:48 -07:00
epriestley
3a87a95e11 Use ManiphestTaskQuery in nearly all interfaces
Summary:
Ref T603. Make almost every task read policy-aware. Notable exceptions are:

  - Edge editor -- this stuff is prescreened and should be moved to ApplicationTransactions eventually anyway.
  - Search/attach stuff -- this stuff needs some general work. The actual list should be fine since you can't pull handles. There may be a very indirect hole here where you could attach an object you can't see (but do know the ID of) to an object you can see. Pretty fluff.
  - The "Tasks" field in Differential will let you reference objects you can't see. Possibly this is desirable, in the case of commandeering revisions. Mostly, it was inconvenient to get a viewer (I think).

Test Plan:
  - Called `maniphest.info`.
  - Called `maniphest.update`.
  - Batch edited tasks.
  - Dragged and dropped tasks to change subpriority.
  - Subscribed and unsubscribed from a task.
  - Edited a task.
  - Created a task.
  - Created a task with a parent.
  - Created a task with a template.
  - Previewed a task update.
  - Commented on a task.
  - Added a dependency.
  - Searched for "T33" in object search dialog.
  - Created a branch "T33", ran `arc diff`, verified link.
  - Pushed a commit with "Fixes T33", verified close.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7119
2013-09-25 13:44:14 -07:00
Chad Little
9be7a948f9 Move PHUIFormBoxView to PHUIObjectBoxView
Summary: I'd like to reuse this for other content areas, renaming for now. This might be weird to keep setForm, but I can fix that later if we need.

Test Plan: reload a few forms in maniphest, projects, differential

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7120
2013-09-25 11:23:29 -07:00
epriestley
119c2b8cec Fix differential.getdiff, etc., for diffs with no Arcanist Project
Summary:
`getArcanistProjectName()` has some logic which gets messy with the `self::ATTACHABLE` mechanism. This makes `differential.getdiff` and similar Conduit methods throw an exception when querying a diff which doesn't have a project. See <http://pastebin.com/Czzrd0Jz>.

Instead, unconditionally attach a project (possibly `null`) when loading diffs if they need projects.

Test Plan: Ran `differential.getdiff` against a `arc diff --raw` diff with no project, got a result instead of an exception.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, sttwister

Differential Revision: https://secure.phabricator.com/D7101
2013-09-24 10:48:40 -07:00
Chad Little
0d77a7f39f ObjectHeader Status icons
Summary: Adds status icons and colors to Maniphest and Differential. Also minor tweaks to them in hovercards. Probably some other stuff too.

Test Plan: Test many diff and task states.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7098
2013-09-24 08:42:04 -07:00
epriestley
806ba1e7a1 Fix a missing setViewer() in differential.getrevision
Summary: DiffQuery now requires this.

Auditors: btrahan
2013-09-23 15:22:57 -07:00
epriestley
b3fa9d0c2f Modernize Diffusion "change" view
Summary:
  - Kicks it out to full width.
  - More useful header/crumbs/properties/actions (needs some more work).
  - Works for public repositories.
  - Fix a bug where the "rX" crumb would lose the branch you're on.

Test Plan: See screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7063
2013-09-23 12:54:12 -07:00
epriestley
e7a7e43104 Fix a bug where policy queries with cursor-based pagers and non-ID orders can go into infinite loops
Summary:
Ref T603. See inlines for an explanation. The case where I hit this was loading the "Pending Differential Revisions" panel in Diffusion when logged out, after making a repository public.

What happens is that we load 10 revisions (say, D1 .. D10) but the user can't see any of them. We then try to load the next 10, but since the pagination is ordered by date modified, we need to base the next query on the modified date of the last thing we loaded (D10). However, since we use the viewer's policies to load that cursor object, it fails to load, and then we just issue the same query over and over again, loading D1 .. D10 until we run out of execution time.

Test Plan: Interface now loads correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7059
2013-09-21 16:23:44 -07:00
epriestley
a025050e87 Fix an issue with differential.getdiff when providing a revision ID
Summary:
If handed a revision ID, we might get more than one result, which causes `executeOne()` to throw. Instead, translate the revision id into a diff ID before querying for the diff.

Also one small consistency change to parameter casing.

Test Plan: Used console to query for a revision with more than one diff using the revision id.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, mbishopim3

Differential Revision: https://secure.phabricator.com/D7026
2013-09-18 15:31:48 -07:00
Chad Little
14aa70a2e0 Only return description in Policy description if no image required
Summary: We were returning an array here when  previous return was a string.

Test Plan: reload diff

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7025
2013-09-18 12:06:07 -07:00
epriestley
cd4cb12116 Minor fix to minor fix to diff order
Summary: We need to preserve keys here; the keys are the diff IDs and are meaningful.

Auditors: btrahan
2013-09-18 11:56:48 -07:00
Joel Beales
df8474d778 Conduit - Add option for createcomment to attach draft inline comments
Summary:
Conduit has a query to make a draft inline comment, but createcomment doesn't have the ability to attach them.
Added optional parameter to attach any existing draft comments. Default value is false, so existing api users won't be effected by the change.

Test Plan: Tested no draft comments and multiple draft comments, attach_inlines =true, false, and empty.

Reviewers: vrana

Reviewed By: vrana

CC: epriestley, aran

Differential Revision: https://secure.phabricator.com/D7019
2013-09-18 10:03:35 -07:00
epriestley
209edcd75a Fix two minor Differential issues
Summary:
  - D6966 accidentally reversed the order of `$diffs`. Reverse it back.
  - The new policy header stuff returns `array(icon, text)` but gets `strlen()`'d by a caller. Silence that warning for now.

Test Plan: Created a revision with several diffs. Saw them in the right order; saw no warning on the diff attach screen.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran, mbishopim3

Differential Revision: https://secure.phabricator.com/D7023
2013-09-18 08:57:16 -07:00
Bob Trahan
52e65f3d47 Add a differential.getdiffs method
Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this.  Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823.

Test Plan: used the new api via test console - great success.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3823

Differential Revision: https://secure.phabricator.com/D6966
2013-09-17 13:55:41 -07:00
Chad Little
e8bb24fd60 Policy, Status in PHUIHeaderView
Summary: The adds the ability to set 'properties' such as state, privacy, due date to the header of objects.

Test Plan: Implemented in Paste, Pholio. Tested various states.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7016
2013-09-17 09:12:37 -07:00
epriestley
b398ae5504 Dispatch Differential edit events from Editor, not Controller
Summary:
Currently, these events don't fire for Conduit updates, which makes them sort of silly.

This will get proper treatment after T2222.

Test Plan: Installed a `throw new Exception(...)` event listener. Performed Conduit and web updates of revisions, saw event listener fire.

Reviewers: btrahan, guywarner

Reviewed By: guywarner

CC: aran

Differential Revision: https://secure.phabricator.com/D7004
2013-09-16 08:04:14 -07:00
epriestley
256fcf3721 Make it easier to construct multi-column paging clauses from Query classes
Summary:
We currently have two giant messes for paging across multiple columns (usually because one column is not unique), and I'm about to add a third for Maniphest.

Provide a more structured way to build these `A > a OR (A = a AND B > b)` clauses.

Test Plan: Set page size to `2` for Differential and Diffusion and paged forward and backward with a bunch of different orders set. Pages worked as expected.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2625

Differential Revision: https://secure.phabricator.com/D6971
2013-09-13 11:49:41 -07:00
epriestley
7a39ac43b4 Add a "list<regex>" config option and move regex config to it
Summary:
Fixes T3807. Several issues:

  - Currently, we split config of type `list<string>` on commas, which makes it impossible to enter a regex with a comma in it.
    - Split on newlines only.
  - Some of the examples are confusing (provided in JSON instead of the format you actually have to enter them).
    - Show examples in the same format you should enter text.
  - We didn't validate regexps.
    - Introduce `list<regex>` to validate regexes.

@hlau: Note that the old config format for the bugtraq stuff implied the delimiters on the regular expression. They are no longer implied. The examples show the correct format.

Test Plan: Viewed and edited affected config, hitting error and success cases.

Reviewers: btrahan

Reviewed By: btrahan

CC: hlau, aran

Maniphest Tasks: T3807

Differential Revision: https://secure.phabricator.com/D6969
2013-09-13 11:48:00 -07:00
Chad Little
85424e7472 Small button dropdowns
Summary: Adds the small caret to differential. Cleans up dropdown frame.

Test Plan: Test caret in differential.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6983
2013-09-13 10:48:02 -07:00
Bob Trahan
ab2ae9e47f Differential - make sure not to return change type header if we're not top level
Summary: Followup to D6924. Fixes T3824.

Test Plan: deleted a file in a diff. was able to view file content without JS errors

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3824

Differential Revision: https://secure.phabricator.com/D6963
2013-09-12 16:00:00 -07:00
epriestley
8f8c61be31 Remove legacy "touched" table and indexing
Summary: Noticed this in the schema. "Touches" were an idea that never really got off the ground, as we built out more/better notification channels instead. Essentially, they recorded any object you'd ever interacted with. Maybe this will be useful some day, but for now it does nothing and can't be interacted with. Nuke it.

Test Plan: `grep`, loaded Maniphest.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D6953
2013-09-12 13:04:09 -07:00
Bob Trahan
b902005bed Kill PhabricatorObjectDataHandle
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))

Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran, FacebookPOC

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6941
2013-09-11 12:27:28 -07:00
Bob Trahan
07b8becfc6 Policy - introduce parentQuery and pass around policy configuration from parent to child
Summary: Ref T603. Ref D6941.

Test Plan: Clicked around all over - looked good. I plan to re-test D6941 to make sure the executeOne case works now as intended

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6944
2013-09-11 12:19:34 -07:00
epriestley
a2571de575 Remove obsolete/deprecated withTaskIDs() / withTaskPHIDs()
Summary: Ref T603. These were deprecated some time ago in favor of the more standard withIDs() / withPHIDs().

Test Plan: `grep`, loaded some interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6929
2013-09-10 15:34:04 -07:00
epriestley
e625c91867 Pass viewer to all ManiphestTaskQuery objects
Summary: Ref T603. Prepare for conversion to a policy-aware query.

Test Plan: Browsed various interfaces which use this stuff.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6928
2013-09-10 15:34:03 -07:00
epriestley
8e45b466da Improve voicing in text published to JIRA issues
Summary:
Ref T3687. JIRA is able to piggyback on a fair amount of Asana infrastructure, but the voicing we use on Asana tasks (which are always about one object) isn't very good for JIRA issues (which may have many linked objects). Specifically, we publish stories like this to Asana:

  alincoln accepted this revision.

This is meaningless in JIRA since you have no idea what it's talking about. Instead, publish like this:

  alincoln accepted D999: Put a bird on it

Additionally, supplement it with a URI, so the total story text we publish is:

  alincoln accepted D999: Put a bird on it

  https://phabricator.whitehouse.gov/D999

Signifcantly less useless!

Test Plan: {F57523} {F57524}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D6907
2013-09-10 15:22:24 -07:00
Chad Little
5ba20b8924 Move PhabricatorObjectItem to PHUIObjectItem, add 'plain' setting for lists.
Summary: Adds plain support for object lists that just look like lists

Test Plan: review UIexamples and a number of other applications

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6922
2013-09-09 14:14:34 -07:00
epriestley
470bb4931b Fix a warning in the JIRA field
Summary: Ref T3687. The `value` property may be `null`.

Test Plan: Loaded a revision with the JIRA field enabled but no issues attached, no longer saw a warning about a bad argument to `foreach()`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D6890
2013-09-05 16:51:13 -07:00
Andrew Gallagher
8bba3f280d Only associate branch with task if maniphest is enabled
Summary:
Previously, maniphest tasks would get upated by diffs on branches
with tasky names, even if maniphest was disabled.

Test Plan:
Tested createing a diff in sandbox with maniphest disabled, on a
git branch named using the format "t###".  Without this change,
if there happened to be a task in the maniphest DB which matched,
it was updated an email was sent to users.

Reviewers: epriestley

Reviewed By: epriestley

CC: wez, slawekbiel, whhone, Korvin, aran

Differential Revision: https://secure.phabricator.com/D6881
2013-09-03 18:22:01 -07:00
epriestley
853544b54a Add "JIRA Issues" field to Differential
Summary:
Ref T3687. This adds a field which allows you to link Differential Revisions to JIRA issues.

This is just about as basic as it can get, but gets the job done. The field enables itself if you have a JIRA auth provide. You enter JIRA issues in a comma-delimited format and it generates appropriate edges.

Nothing is pushed to the issues yet.

The only real rough part here is that if you commandeer a revision which is linked to issues you can't see, editing it is difficult via the CLI. This seems pretty much like a non-issue, but at some point we can let the field throw some kind of "RecoverableInvalidFieldException" which just warns the user. The "no reviewers, continue anyway?" prompt could then use that too.

Test Plan:
  - Edited via web UI, tried valid/invalid edits, checked that edges showed up in the database, added/removed issues, clicked issue links.
  - Edited via CLI, tried valid/invalid edits.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D6879
2013-09-03 17:27:51 -07:00
Gareth Evans
fcba0c74d9 Replace all "attach first..." exceptions with assertAttached()
Summary:
Ref T3599
Go through everything, grep a bit, replace some bits.

Test Plan: Navigate around a bit

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3599

Differential Revision: https://secure.phabricator.com/D6871
2013-09-03 06:02:14 -07:00
epriestley
eb3690f8c6 Add "Authored" as a default filter to Differential
Summary: Fixes T3786. Not 100% sold on this (I don't want to restore all of the original filters, since users can and should just build the weird ones if they use them), but this is almost certainly the most useful of the defaults which ApplicationSearch removed.

Test Plan: Viewed `/differential/`, executed the query.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3786

Differential Revision: https://secure.phabricator.com/D6860
2013-09-01 19:15:52 -07:00
Wez Furlong
11f1268e99 don't throw BadMethodCallException on diffs that add images
Summary:
I don't know if there is something more sinister going on
under the covers, but we have a couple of diffs that trigger:

Unhandled Exception ("BadMethodCallException")
Call to a member function getMetadata() on a non-object

when the diff page is handling its async render calls.  One diff
in particular has multiple image adds and thus has a stack of of these
error dialogs to close.

This isn't a new regression, we just haven't gotten around to debugging
it until now (reported on 6/12)

One revision that triggers it has two diffs.  If I show Base -> Diff 1
I don't hit the error.  When I select Base -> Diff 2, or Diff 1 -> Diff
2, the error triggers.

I don't understand what this means, but this diff avoids the null object
reference that causes the exception.

Test Plan:
Load the offending diff, don't hit the error. The diff loads
the images that were added

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6851
2013-08-30 08:15:10 -07:00
epriestley
63dbccb8a1 Fix default order for Differential queries
Summary:
Fixes T3781. The UI defaults to "Created" but the query defaults to "Modified". Make the two consistent.

In particular, an issue this fixes is that previously a `/differential/?authors=duck` page would show "Order: Created" but actually order by "Modified".

Test Plan: Visited `/differential/?authors=duck` and verified the revisions were ordered by creation date.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3781

Differential Revision: https://secure.phabricator.com/D6843
2013-08-29 15:07:30 -07:00
epriestley
f1c75a6382 Allow construction of ApplicationSearch queries with GET
Summary:
Ref T3775 (discussion here). Ref T2625.

T3775 presents two problems:

  # Existing tools which linked to `/differential/active/epriestley/` (that is, put a username in the URL) can't generate search links now.
  # Humans can't edit the URL anymore, either.

I think (1) is an actual issue, and this fixes it. I think (2) is pretty fluff, and this doesn't really try to fix it, although it probably improves it.

The fix for (1) is:

  - Provide a helper to read a parameter containing either a list of user PHIDs or a list of usernames, so `/?users[]=PHID-USER-xyz` (from a tokenizer) and `/?users=alincoln,htaft` (from an external program) are equivalent inputs.
  - Rename all the form parameters to be more digestable (`authorPHIDs` -> `authors`). Almost all of them were in this form already anyway. This just gives us `?users=alincoln` instead of `userPHIDs=alincoln`.
  - Inside ApplicationSearch, if a request has no query associated with it but does have query parameters, build a query from the request instead of issuing the user's default query. Basically, this means that `/differential/` runs the default query, while `/differential/?users=x` runs a custom query.

Test Plan: {F56612}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2625, T3775

Differential Revision: https://secure.phabricator.com/D6840
2013-08-29 11:52:29 -07:00
epriestley
5a11f08ba4 Use bar colors to show revision status in revision lists
Summary:
Ref T3772. The original version of D5451 had a very colorful version of this which felt a bit arbitrary, and we moved away from it after discussion, particularly [[ https://secure.phabricator.com/D5451#comment-8 | here (chad) ]] and [[ https://secure.phabricator.com/D5451#comment-14 | here (me) ]] and [[ https://secure.phabricator.com/D5451#comment-19 | here (chad again) ]].

The core of my objection was that status and priority to the viewer aren't the same: a "needs revision" revision that you authored is high priority (you need to revise it), but a "needs revision" revision that someone else authored is low priority (you're waiting on them to revise it). If we color by status, revisions in both high priority and low priority states will be colored red. We can instead color by viewer priority (blocking others = red, needs attention = orange, waiting on others = blue; or something), but that would be redundant (we already group by it, so you'd get big chunks of stuff with the same color and color would have no utility), confusing (in ungrouped views, the colors would not be self-explanatory) and weirdly inconsistent (different users would see objects having different colors).

I still think all this holds, but I also thought that "viewer priority" was enormously more important than "state", since I use the former frequently and the latter very rarely. From T3772, it sounds like some users use "state" a lot more than I do (i.e., they want to find "accepted" revisions within a "viewer priority" group like "Action Required"). This is a possible approach to that.

I think another issue was the heavy use of the color in the original; this restores a more conservative version of it which doesn't have as much weight. In particular:

  - Revisions in the "Needs Review" state retain the default color, rather than orange.
  - Revisions in the "Closed" state have the disabled effect.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T3772

Differential Revision: https://secure.phabricator.com/D6839
2013-08-29 09:16:51 -07:00
epriestley
d06129b81e Add setEpoch() and onboard staleness to ObjectItemListView
Summary: Fixes T3486. I don't love how this looks -- maybe we could try different icons? Like white icons on a brighter red/yellow background?

Test Plan: {F56299}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T3486

Differential Revision: https://secure.phabricator.com/D6833
2013-08-28 16:48:42 -07:00
epriestley
86455b8591 Allow disabled users to be typeaheaded in Differential
Summary: Fixes T3773. By default, the `/users/` datasource excludes disabled users (since it doesn't make sense to assign them tasks or make them reviewers, for example). However, for ApplicationSearch it does make sense to look for objects, e.g., authored by a disabled user.

Test Plan: Searched for disabled users in Differential.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3773

Differential Revision: https://secure.phabricator.com/D6834
2013-08-28 15:37:53 -07:00
Wez Furlong
7b5471b1ae fix perf regression when using 'owned' query
Summary:
D6335 has some unexpected side effects.  This adds back the
where clause for the owned query.  There may be other problems.

Test Plan:
Ran:

```
echo '{"query":"owned","guids":["myphid"]}' | arc --conduit-uri=https://myhost call-conduit differential.find
```

Reviewers: epriestley, dschleimer

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6832
2013-08-28 13:29:36 -07:00
Chad Little
bb9be01d55 Update forms to use PHUIFormBoxView
Summary: Some more callsites, let me know if you see others, I think think is 98% of them now.

Test Plan: tested each page

Reviewers: epriestley, btrahan

Reviewed By: btrahan

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6814
2013-08-26 15:45:58 -07:00
Chad Little
fe2a96e37f Update Form Layouts
Summary:
This attempts some consistency in form layouts. Notably, they all now contain headers and are 16px off the sides and tops of pages. Also updated dialogs to the same look and feel. I think I got 98% of forms with this pass, but it's likely I missed some buried somewhere.

TODO: will take another pass as consolidating these colors and new gradients in another diff.

Test Plan: Played in my sandbox all week. Please play with it too and let me know how they feel.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6806
2013-08-26 11:53:11 -07:00
epriestley
f034fd80db Remove getApplicationObjectTypeName from ApplicationTransactions
Summary:
We can get this out of PHIDType reasonably in all cases and simplify implementation here.

None of these translate correctly anyway so they're basically debugging/development strings.

Test Plan: `grep`, browsed some transactions

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D6786
2013-08-21 12:32:06 -07:00
epriestley
751cd547c2 Remove dust from page construction
Summary:
  ^\s+(['"])dust\1\s*=>\s*true,?\s*$\n

Test Plan: Looked through the diff.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6769
2013-08-19 18:09:35 -07:00
Bob Trahan
d8a1e7e15f Differential - add an undo element when you collapse a file
Summary: Fixes T2258.

Test Plan: collapsed and expanded file via the dropdown - good stuff. got the "undo" element into the mix - also good stuff.

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, Korvin, aran

Maniphest Tasks: T2258

Differential Revision: https://secure.phabricator.com/D6742
2013-08-13 16:05:09 -07:00
epriestley
796007a85e Publish inline comments in Asana notification stories
Summary: Ref T2852. Bleh, gross. Does what it says in the title.

Test Plan: {F54024}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6735
2013-08-13 10:16:56 -07:00
Jakub Vrana
8c93e3c941 Allow using colon in Depends On
Test Plan: Used it.

Reviewers: epriestley

Reviewed By: epriestley

CC: tdrhq, aran, Korvin

Differential Revision: https://secure.phabricator.com/D6705
2013-08-08 11:43:25 -07:00
epriestley
6badb05d64 Make Herald adapters provide content types
Summary:
Ref T2769. Get content types out of hard-coded config and into dynamic adapters.

This removes the "MERGE" and "OWNERS" content types, which were vestigal. These needs are likely better addressed through subscriptions/transactions, and are obsolete, and haven't existed for 2+ years and no one has asked for them to be restored.

Test Plan: Mostly a bunch of grep. Viewed rule list, rule edit. Edited a revision.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6656
2013-08-07 18:03:51 -07:00
epriestley
aa8c661d5d Don't publish story text for "close" stories to Asana
Summary: Ref T2852. After some discussion, Asana doesn't want "close" stories either.

Test Plan: Used `bin/feed republish` to publish close and non-close stories from Differential and Diffusion. Verified comments were synchronized in the expected cases.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6697
2013-08-07 13:28:58 -07:00
Kieran Brownlees
8b07c498d4 Add NEEDS_REVISION support to DifferentialRevisionQuery
Reviewed by: epriestley

See: https://github.com/facebook/phabricator/pull/370
2013-08-07 08:00:32 -07:00
epriestley
193a9611e4 Partially generalize Remarkup previews and add support to Differential
Summary:
Ref T3671. A lot of applications have pretty ad-hoc preview code. Clean it up a bit and add Summary preview to Differential.

After ApplicationTransactions we might want to try to serialize the whole form and show a preview of all the transactions, but this seems not very useful in most cases (I'd guess that Remarkup previews are 99% of the value) and tricky to get right (e.g., adding images which don't exist yet to Pholio mocks).

I think I can add this in a few other places, too.

Test Plan:
Edited Maniphest Tasks and Differential Revisions, mashed some buttons. Verified previews rendered correctly. Grepped for removed CSS classes (no hits).

{F52907}

Reviewers: btrahan, Firehed

Reviewed By: btrahan

CC: aran, chad

Maniphest Tasks: T3671

Differential Revision: https://secure.phabricator.com/D6673
2013-08-05 10:46:39 -07:00
epriestley
75e56cb25d Publish create object stories into Asana sort of, but not really
Summary: Ref T2852. Current code works fine, but although we want to drop creation stories, we really only want to drop the story text, not the other effects of the creation story. Also generalize this mechanism so we don't have Asana-specific code in the publishers.

Test Plan: Used `bin/feed republish` to publish creation and non-creation stories. Verified creation story published no text.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6639
2013-08-01 12:19:18 -07:00
epriestley
091beee730 Don't publish Differential "create" action to Asana
Summary: Ref T2852. Currently there's effectively a double notification: one for creating the task, and one for the "alincoln created this revision" story. Drop the "create" story.

Test Plan: Used `bin/feed republish` to republish "create" and non-"create" stories. Verified "create" was dropped as unsupported, while non-"create" went through.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6589
2013-07-27 16:33:47 -07:00
Bob Trahan
1cb0db8755 Move PhabricatorUser to new phid stuff
Summary: Ref T2715. Had to start loading status information in the query class. Debated trying to clean up some of the attach / load stuff but decided to just add status under the new paradigm for now.

Test Plan: phid.query  also made a status and checked that out. also played in conpherence.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2715

Differential Revision: https://secure.phabricator.com/D6585
2013-07-26 14:05:19 -07:00
epriestley
3ba2f506fe Remove leading whitespace if no prefix is configured for Asana sync
Summary: Ref T2852. If the prefix is removed by configuration, we'll incorrectly leave a leading space. Trim any leading whitespace off.

Test Plan: Ran `bin/feed republish` to sync an object to Asana

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6575
2013-07-26 11:37:48 -07:00
epriestley
8514a3c739 Generalize Asana-publishable feed story objects
Summary: Ref T2852. Pulls the Differential-specific aspects of the Asana sync out of the worker. Next diff will add a publisher for Audit/Diffusion.

Test Plan: Published events, including state changes. Saw them reflected correctly in Asana.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6569
2013-07-26 08:56:35 -07:00
epriestley
9383abc6b0 Remove unnecessary empty checks from willFilterPage()
Summary: Fixes T3600. These checks are obsolete after D6512.

Test Plan: Syntax / static / inspection.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3600

Differential Revision: https://secure.phabricator.com/D6563
2013-07-24 15:30:26 -07:00
epriestley
f7e1450096 Fix "setName()" on Differental revision handles
Summary: I copied this over wrong. Ref T2715

Test Plan: Looked at Maniphest with linked commits/revisions, e.g.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2715

Differential Revision: https://secure.phabricator.com/D6531
2013-07-22 14:42:31 -07:00
epriestley
cf255cde3d Fix "request" and "update" mail preferences for Differential
Summary:
Fixes T3030. T1977 attempted to fix this but either didn't work (I think this is the case) or was broken later. We don't send `DifferentialCommentMail` on a create or update; we send `DifferentialReviewRequestMail`.

Also update the details to be more clear.

Test Plan:
Verified review request mail is marked undeliverable:

```
$ ./bin/mail show-outbound --id 6644
...
PARAMETERS
...
mailtags: ["differential-review-request"]
...
subject: D922: asdf
subject-prefix: [Differential]
vary-subject-prefix: [Request, 100 lines]
...
RECIPIENTS
! duck (duck)
    - This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences).

BODY
epriestley requested code review of "asdf".
...
```

Verified update mail is marked undeliverable:

```
$ ./bin/mail show-outbound --id 6646
...
Message: Message has no valid recipients: all To/Cc are disabled, invalid, or configured not to receive this mail.

PARAMETERS
...
mailtags: ["differential-updated"]
...
subject: D922: asdf
subject-prefix: [Differential]
vary-subject-prefix: [Updated, 100 lines]
...
RECIPIENTS
! duck (duck)
    - This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences).

BODY
epriestley updated the revision "asdf".
...
```

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3030

Differential Revision: https://secure.phabricator.com/D6518
2013-07-22 12:20:48 -07:00
epriestley
1fb39a20d3 Move DifferentialRevision to application PHIDs
Summary: Ref T2715.

Test Plan: Used `phid.lookup` and `phid.query` to load handles. Grepped for `PHID_TYPE_DREV`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2715

Differential Revision: https://secure.phabricator.com/D6509
2013-07-22 12:17:29 -07:00
epriestley
7492eb36d4 Fix missing label in Request Review
Summary: Fixes T3564. This was a change out of FB recently, see D6340. Add a missing label.

Test Plan: "Request Review" now has a label.

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3564

Differential Revision: https://secure.phabricator.com/D6484
2013-07-17 11:37:54 -07:00
epriestley
fca534d6b6 Improve Asana API error handling in Doorkeeper
Summary:
Ref T2852. We need to distinguish between an API call which worked but got back nothing (404) and an API call which failed.

In particular, Asana hit a sync issue which was likely the result of treating a 500 (or some other error) as a 404.

Also clean up a couple small things.

Test Plan: Ran syncs against deleted tasks and saw successful syncs of non-tasks, and simulated random failures and saw them get handled correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6470
2013-07-16 10:29:52 -07:00
Juan Pablo Civile
ee9fac5c8f Use DifferentialRevisionQuery in differential controllers
Summary:
Change all instances of `id(new DifferentialRevision())->load($id)` for `DifferentialRevisionQuery` where reviewers are loaded.
Also make sure that the new reviewer status is being loaded so that all calls to `getReviewers` can be removed in the near future.

Test Plan: Use all three controllers with several revisions and check they still work in sane way

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D6466
2013-07-15 16:01:31 -07:00
Juan Pablo Civile
d6ce5a31e3 Use DifferentialRevisionQuery in conduit when reviewers are needed
Summary:
Ref T1279.

Switched all differential conduit methods to use `DifferentialRevisionQuery` where `loadRelations` was being used.

Test Plan: Called all the methods changed and verified they still worked as advertised.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T603, T1279

Differential Revision: https://secure.phabricator.com/D6452
2013-07-15 03:29:13 -07:00
Juan Pablo Civile
d4c28dcbc2 Methods for reading reviewers from edges in differential
Summary: Add `getReviewerStatus` to get an array of `DifferentialReviewer` objects. The method `needReviewerStatus` in `DifferentialRevisionQuery` loads the edges into the revisions loaded.

Test Plan: Added `->needReviewerStatus(true)` to `DifferentialRevisionSearchEngine` and checked through logging that the data was being loaded correctly.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6450
2013-07-14 19:18:56 -07:00
Bob Trahan
e4725832c4 Clean up some more carnage from D6416
Summary: rPad17c99c1b0222292a47ca79561a356cb8b5a5d5 stopped the fatal and this provides the forward fix. I think this is what a forward fix is anyway.

Test Plan: viewed a revision (D63 is my boy) and no fatals

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6448
2013-07-13 10:33:32 -07:00
epriestley
ad17c99c1b Fix fatal on Differential revision view
Auditors: btrahan
2013-07-12 22:38:48 -07:00
Bob Trahan
9838251515 Make PhabricatorActionListView logged-out user savvy
Summary:
Fixes T2691. Now, all PhabricatorActionListViews in the codebase setObjectHref to $request->getRequestURI. This value is passed over to PhabricatorActionItems right before they are rendered. If a PhabricatorActionItem is a workflow and there is no user OR the user is logged out, we used this objectURI to construct a log in URI.

Potentially added some undesirable behavior to aggressively setUser (and later setObjectURI) from within the List on Actions... This should be okay-ish unless there was a vision of actions having different user objects associated with them. I think this is a safe assumption.

Test Plan: played around with a mock all logged out (Ref T2652) and it worked!

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2691

Differential Revision: https://secure.phabricator.com/D6416
2013-07-12 11:39:47 -07:00
Juan Pablo Civile
fb9282452b Fix typo in D6372
Summary: One place used status, other used state. Killed state in favor of status.

Test Plan: None at all

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6422
2013-07-10 16:26:38 -07:00
Juan Pablo Civile
70fd3dd54f Store revision reviewer state as edges
Summary:
Keep track of the state of a reviewer in an edge between reviewer and revision.
The edge stores the state of the review, added or rejected. And if the revision was
accepted by that reviewer the id of the diff accepted.

Test Plan:
Create diffs and clowncopterize reviewer list changes. This includes:
 * Adding new reviewers
 * Resigning
 * Commandering a revision

Reviewers: epriestley

CC: aran, Korvin

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D6372

Conflicts:
	src/applications/differential/editor/DifferentialCommentEditor.php
2013-07-10 13:50:21 -07:00
epriestley
a77ab312f0 Use PHUIIconView for PhabricatorActionView
Summary: Allows application icons to appear in action lists.

Test Plan: {F49487}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6408
2013-07-10 12:33:51 -07:00
Bian Jiang
c977168797 Show add reviewer typehead when user selects resign as a reviewer.
Summary:
1. Show add reviewer typehead when user selects resign as a reviewer.
2. Change the label for add reviewers typehead when user selects resign as a reviewer.

Test Plan:
1. Add yourself as a reviewer in a diff.
2. Select "Resign as Reviewer" in comment editor.
Add reviewer typehead should display, with label "Suggest Another Reviewer".
Add reviewer typehead is also displayed after user refreshed the page with "Resign as Reviewer"
selected.

Reviewers: wez, epriestley

Reviewed By: epriestley

CC: aran, epriestley, akramer, person

Differential Revision: https://secure.phabricator.com/D6340
2013-07-10 11:05:53 -07:00
epriestley
6a40df529d Use ActionListView for all profile actions
Summary:
This leaves the space between the properties and the blurb looking a bit empty, but there will be more stuff there soon (status, VCS names, email, phone/fax numbers, etc., and custom user fields).

I removed "view lint messages" since I'm pretty sure no one has ever clicked it. I think providing better search (e.g, T2625) to that UI in Diffusion is a preferable approach.

Test Plan: {F49423}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6403
2013-07-10 05:11:08 -07:00
Lauri-Henrik Jalonen
93e37e9060 Phabricator event timeline removed
Summary: Removed related files and references

Test Plan: Crossed my fingers

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, AnhNhan

Maniphest Tasks: T2003

Differential Revision: https://secure.phabricator.com/D5744

Conflicts:
	src/__phutil_library_map__.php
	src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
2013-07-09 18:07:42 -07:00
epriestley
13e2489739 Add a link to the main Asana task from Differential
Summary: Ref T2852. When a Differential revision is linked to an Asana task, show the related task in Differential.

Test Plan: {F49234}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6387
2013-07-09 16:22:33 -07:00
epriestley
64cc0ce128 Add "Visible To" property fields for diffs and revisions
Summary: Ref T603. Show object visibility in the UI. This isn't editable or mutable yet, but will be after T2222.

Test Plan: {F48689} {F48690}

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6361
2013-07-03 11:29:57 -07:00
epriestley
ebfa7e2c95 Make UX improvements to diff create screen
Summary: Minor tweaks for consistency, and raise a friendlier error if the user doesn't upload anything.

Test Plan:
{F48686}
{F48685}

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6360
2013-07-03 10:32:02 -07:00
epriestley
29658db32e Fix margins and spacing of other revision lists
Summary: Fixes spacing in Differential revision detail and Diffusion browse views.

Test Plan:
{F48677}
{F48678}

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6359
2013-07-03 10:10:07 -07:00
epriestley
82b37b9fd8 Show draft icon in attributes?
Summary: Ref T3485. Maybe this is OK?

Test Plan: {F48656}

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3485

Differential Revision: https://secure.phabricator.com/D6356
2013-07-03 08:53:01 -07:00
epriestley
197aa43648 Move flag icon inline with header in Differential revision list
Summary: Ref T3485. Moves flag icon inline in the header.

Test Plan: {F48654}

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3485

Differential Revision: https://secure.phabricator.com/D6355
2013-07-03 08:52:53 -07:00
epriestley
0407b22ea2 Move "Stale" / "Old" to date icon in Differential list view
Summary: Ref T3485. Moves "Stale" / "Old" to the right.

Test Plan: {F48653}

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3485

Differential Revision: https://secure.phabricator.com/D6354
2013-07-03 08:52:46 -07:00
epriestley
6aee862bbe Use ApplicationSearch in Differential
Summary:
Ref T603. Ref T2625. Fixes T3241. Depends on D5451. Depends on D6346.

@wez, this changes the Differential revision list UI substantially and may generate a lot of bikeshedding / who-moved-my-cheese churn. See T3417 for context, for example. The motivations for this change are:

  - The list now works on devices, like phones and tablets. This is a requirement to make the rest of Differential work on devices.
  - Although ApplicationSearch intentionally presents a simpler interface initially and some options which were one click away before aren't now, it is much more powerful than the search it replaces and allows users to build, save, share, fork, edit, and customize a much wider range of queries. Users who used the old filters frequently can use Advanced Search -> Save Custom Query to create new versions of them, and of any other query. "Edit Queries.." allows users to remove and reorder queries, including builtin queries. Basically, there are like three things which have gone from "1-click" to "a few clicks", and ten trillion things which have gone from "hard/impossible" to "relatively easy".

The local screenshots look a bit iffy, but I think a lot of this is the fakenesss of my test data. If they still feel iffy in production we can tweak them until they feel good, like we did for Maniphest.

Test Plan:
{F48477}

{F48478}

Reviewers: btrahan, chad, wez

Reviewed By: btrahan

CC: aran, s

Maniphest Tasks: T603, T2625, T3241

Differential Revision: https://secure.phabricator.com/D6347
2013-07-03 06:11:07 -07:00
Anh Nhan Nguyen
d9f01d6fb7 [Rough Sketch] Differential ObjectItemView Smexyness
Summary:
Tried out `PhabricatorObjectItemView` for Differential. It looks smexy and smooth.

Refs T2014

- Title and Date as Maniphest
- Author in the handle icon
- Bar color reflects revision status (Needs Review, Accepted, Abandoned etc.) @chad looking for non-blue is faster than keeping watch for everything that's not "Closed" in old table form
- Some status information are in footer icons; currently only stale/old status display as well as saved drafts, maybe more in future; these come into my mind:
  - No reviewer warning
  - Push Blocking Priority (T2730)
  - Trivial, fast review guaranteed
  - Sketch / Just looking for advice/help
  - Arcanist Project (T2614)
  - Denote "Public Send-in" (T1476)

{F37662}
{F37663}
{F37664}
{F37665}

Some flaws:

- Date and reviewers on every entry the same?
- No respect for Differential fields (for some reason, every entry appeared the same, so broke it to parts)
- Plenty of (potential) increase in height - advise reducing paging length from 100 to 50 - or just ignore me

Suggestions for the future:

- Expand the meta information regarding revisions; e.g. the various status displays above
- Uh... T2543, T1279, T793, T731 and what else I want for Differential, because they are awesome!
- T793 should be in particular easy appearance-wise, just copy-paste from Maniphest

Test Plan: By looking at it, of course. Verified there are no errors or crashed

Reviewers: epriestley, chad, btrahan, liguobig

Reviewed By: chad

CC: aran, Korvin, edward, nh

Maniphest Tasks: T2014

Differential Revision: https://secure.phabricator.com/D5451

Conflicts:
	src/__celerity_resource_map__.php
2013-07-03 06:10:39 -07:00
epriestley
3ec4984f27 Use cursor-based paging in Differential
Summary:
Ref T603. Ref T2625. Use cursors to page Differential queries, not offsets.

The trick here is that some queries are ordered. In these cases, we either need to pass some kind of tuple or do a cursor lookup. For example, if you are viewing revisions ordered by `dateModified`, we can either have the next page be something like:

  ?afterDateModified=2398329373&afterID=292&order=modified

...or some magical token:

  ?afterToken=2398329373:292&order=modified

I think we did this in Conpherence, but one factor there was that paging orders update with some frequency. In most cases, I think it's reasonable to pass just the ID and do a lookup to get the actual clause value (e.g., go look up object ID 292 and see what its dateModified is) and I think this is much simpler in general.

Test Plan: Set page size in Differential to 3, and paged through result lists ordered by date created and date modified.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603, T2625

Differential Revision: https://secure.phabricator.com/D6345
2013-07-03 05:45:07 -07:00
epriestley
0c2e38e81c Make DifferentialRevisionQuery policy-aware
Summary:
Ref T603. Ref T2625. Makes `DifferentialRevisionQuery` do policy checks.

Note that it still uses inefficient offset-based paging, but it's rare to page through revisions. I'll switch to cursor paging in a future diff.

Test Plan: Viewed a bunch of Differential interfaces, home page, etc. This shouldn't actually materially impact anything.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603, T2625

Differential Revision: https://secure.phabricator.com/D6344
2013-07-03 05:43:52 -07:00
epriestley
58884b94dc Simplify construction and execution of Differential queries for "responsible" users
Summary:
Currently, when querying for responsible users (revisions where a given user is either the author or a reviewer), we do this:

  - If the query passes a bunch of hard-coded special cases, use a special hard-coded UNION.
  - Otherwise, use a very complicated JOIN/WHERE clause.

This is bad for several reasons:

  - Tons and tons of hard-coding and special casing.
  - The JOIN/WHERE clause performs very poorly for large datasets.
  - (As a material consequence, the homepage issues a responsible query which barely misses the hard-coded special cases and goes down the slow path.)

Instead, //always// use the UNION strategy to execute a "responsible" query. Specifically, if we have responsible PHIDs, temporarily add them to the author list and build a normal query, then repeat for reviewers, then UNION any clauses we built.

Fixes T3377. Ref T603. Ref T2625. Depends on D6342.

There's various folklore about UNION ALL / UNION DISTINCT performance. UNION DISTINCT is simpler here and the number of rows is small, although we could use UNION ALL in the form:

  SELECT * FROM ((SELECT ...) UNION ALL (SELECT ...) ORDER) GROUP LIMIT

...if we find that there's some performance benefit at some point.

Test Plan: Used DarkConsole to examine queries. Viewed home page and Differential dashboard/authors/subscribers.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603, T2625, T3377

Differential Revision: https://secure.phabricator.com/D6343
2013-07-03 05:39:09 -07:00
epriestley
90123dd739 Add DifferentialDiffQuery and change most callsites
Summary:
Ref T603. This introduces a policy-aware DifferentialDiffQuery and converts most callsites.

I've left unusual callsites (mostly: hard to get the viewer, unusual query, queries related to active diffs) alone for now, so this isn't exhaustive but hits 60-80% of sites.

Test Plan: Created diff; created revision; viewed diffs and revisions; made additional conduit calls.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6338
2013-07-01 12:38:42 -07:00
epriestley
328aa383e4 Always provide a viewer when executing DifferentialRevisionQuery
Summary: Ref T603. This query isn't policy-aware yet, but prepare for it to be one day.

Test Plan: Looked at: home page; differential home; differential detail; diffusion browse. Made differential.query conduit call.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6337
2013-07-01 12:38:27 -07:00
epriestley
ab2ed06c38 Remove DifferentialRevisionListData
Summary: Ref T603. This is a very old, very bad version of `DifferentialRevisionQuery`. I want to modernize only the latter. Express the remaining callsite of the former in terms of `DifferentialRevisionQuery`.

Test Plan: Executed all four modes of `differential.find`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6335
2013-07-01 12:38:08 -07:00
epriestley
b28ceafa38 Update Differential diff view
Summary:
Ref T603.

  - Primarily, this gets rid of a `DifferentialRevisionListData` callsite.
  - Also modernize and clean up some UI stuff.

Test Plan:
{F48260}
{F48261}

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6334
2013-07-01 12:37:54 -07:00
epriestley
eb49d8a52b Construct diffs with attached changesets, even if empty
Summary: See discussion in IRC. Not 100% sure what's going on here because of email ghost theives, but conceivably a commit with no changes will end up with `null` changesets instead of `array()` changesets, which throws. Such diffs are certianly possible (`git commit --allow-empty`) even if they aren't the issue in this specific case. See T3416. Initialize changesets to `array()` to avoid throwing.

Test Plan:
Viewed some commits?

iiam

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6339
2013-07-01 09:02:55 -07:00
Jakub Vrana
0529acd05b Fix typo in comment 2013-06-28 09:40:00 -07:00
epriestley
7c2f6f8361 Simplify selection of inline comments from RevisionView
Summary: Ref T2222. Currently, we load inline comments by `commentID` here, but we always pass every commentID associated with the revision. Instead, just load non-draft comments by revision ID. This simplifies querying a little bit and is likely faster anyway (draft comments are currently loaded separately).

Test Plan: Looked at some revisions and verified inlines showed up correctly and in the right places.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6270
2013-06-24 11:01:51 -07:00
epriestley
3124838d65 Undo D6266 (DifferentialComment PHID migration)
Summary:
Ref T2222. My path forward here wasn't very good -- I was thinking I could set `transactionPHID` for the inline comments as I migrated, but it must be unique and an individual DifferentialComment may have more than one inline comment. Dropping the unique requirement just creates more issues for us, not fewer.

So the migration in D6266 isn't actually useful. Undo it -- this can't be a straight revert because some installs may already have upgraded.

Test Plan: Ran new migrations, verified the world ended up back in the same place as before (made comments, viewed reivsions).

Reviewers: btrahan

Reviewed By: btrahan

CC: wez, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6269
2013-06-24 11:00:35 -07:00
epriestley
d0da409eb0 Fix a mistakenly translated query from D6262
Summary: Ref T2222. I didn't translate this query properly; reproduce the original.

Test Plan: When viewing a revision with non-draft inline comments by a user other than the viewer, the inline comments now appear on the changesets themselves.

Reviewers: kawakami, btrahan, garoevans

Reviewed By: garoevans

CC: aran, mbishopim3

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6281
2013-06-24 07:42:37 -07:00
Gareth Evans
35fae8f452 Fix foreach key named same as arr
Summary: I assume this is a bug!

Test Plan: Look at it

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6279
2013-06-23 16:03:11 -07:00
epriestley
75fa580f3f Add PHIDs to DifferentialComments
Summary:
Ref T2222. This adds PHIDs to all Differential comments so I can migrate the inlinecommment table to transaction_comment in the next diff.

@wez, this will issue a few million queries for Facebook (roughly, one for each Differential comment ever made). It's safe to skip the `.php` half of the patch, bring Phabricator up normally, and then apply this patch with Phabricator running if that eases the migration, although the next few diffs will probably be downtime-required migrations so maybe it's easier to just schedule some downtime.

Test Plan: Ran migration locally. Verified existing comments and new comments received PHIDs.

Reviewers: btrahan

Reviewed By: btrahan

CC: wez, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6266
2013-06-21 18:41:14 -07:00
epriestley
6a2ae07791 Abstract access to DifferentialInlineComment behind a Query
Summary:
Ref T2222. See D6260.

Push all this junk behind a Query so I can move the storage out from underneath it.

Test Plan: Viewed home page, list view, revision. Made draft, looked at preview, submitted draft, viewed inline, replied to inline.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6262
2013-06-21 12:54:56 -07:00
epriestley
44302d2f07 Add storage for new Differential transactions and transaction comments
Summary:
Ref T2222. Ref T1460. Depends on D6260.

This creates the new tables, but doesn't start using them. I added three new fields for {T1460}, to represent fixed/done/replied states.

Test Plan: Ran storage upgrade.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T1460, T2222

Differential Revision: https://secure.phabricator.com/D6261
2013-06-21 12:54:29 -07:00
epriestley
6a2e27ba8d Put all DifferentialComment loading behind DifferentialCommentQuery
Summary:
Ref T2222.

I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.

I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:

  - Wrap the new objects in the old objects for reads/writes.
  - Migrate all the existing data to the new table.
  - Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.

This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:

  - The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
  - The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
  - The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.

This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.

Test Plan: Viewed a revision; made revision comments

Reviewers: btrahan

Reviewed By: btrahan

CC: edward, chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6260
2013-06-21 12:51:18 -07:00
Chad Little
7598330e24 Remove subscribe icons
Summary: Used more logical icons for subscribe, auto, and delete instead of the mail icons. Fixes T3329

Test Plan: Tested subscribing and unsubscribing in Maniphest.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T3329

Differential Revision: https://secure.phabricator.com/D6151
2013-06-06 15:06:08 -07:00
Chad Little
f1bf27959f PHUIList, PHUIDocument updates
Summary:
This diff covers a bit of ground.

- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView

Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.

Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6130
2013-06-05 08:41:43 -07:00
epriestley
5d1f94ac8a Fix some Phabricator lint warnings
Summary: Lint.

Test Plan: Lint.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6127
2013-06-04 15:28:24 -07:00
Jakub Vrana
2427d317b2 Reference task based on branch name in arc diff
Summary:
This was mentioned in T2928 and nobody objected.
It just references the task instead of fixing it as that would be too aggressive.
It also doesn't check assignee of the task (by purpose).

Test Plan: Created diff from a branch named T2928.

Reviewers: epriestley, edward

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2928

Differential Revision: https://secure.phabricator.com/D5640
2013-05-30 17:33:17 -07:00
Chris Bolt
5227a06e3c Make Differential's username regex match People's username regex. Refs T3264.
Test Plan: Tested on our local phabricator install.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T3264

Differential Revision: https://secure.phabricator.com/D6066
2013-05-28 15:06:14 -07:00
Gareth Evans
ef797494ca Add Allowed uris config
Summary:
Kind of a quick look at an idea for T2184

Ref T2184

Test Plan: Make sure the site still loads

Reviewers: epriestley

CC: aran, Korvin, mbishopim3

Maniphest Tasks: T2184

Differential Revision: https://secure.phabricator.com/D6045
2013-05-26 10:57:45 -07:00
Afaque Hussain
473a2c3b31 Added willEditTask and didEditTask for Differential.
Summary: Added constants to PhabricatorEventType. Modified DifferentialRevisionEditor and DifferentialCommentEditor.

Test Plan:
Created a revision. Edited and made a comment on that revision. It's updating as usual. I think nothing broke may be it's working.
Let me know if I have done it correclty.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, AnhNhan

Maniphest Tasks: T2899

Differential Revision: https://secure.phabricator.com/D5869
2013-05-24 06:38:54 -07:00
Bob Trahan
a56ca7a043 Conpherence - kill the image-based header UI
Summary:
removes the whole custom image thing, instead using a more standard application crumbs. Gives this glorious space back to the compose area which is now tens of pixels taller. Also defaults it to the people widget. Basically, fixes T3160.

For now, you **CAN NOT** edit the title of a conpherence. I didn't want to jam in too much here. Next diff will be to change the widget icons into the dropdown switcher, which will also bring back the editing of titles.

Test Plan: looked at conpherence and it was pretty. Resized it vigorously and it wasn't too bad.

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T3160

Differential Revision: https://secure.phabricator.com/D5998
2013-05-22 16:05:47 -07:00
epriestley
2214f96d3f Fix some small notification / token issues
Summary:
Fixes T3218.

  - Currently, Paste pages don't clear notifications about the paste (notably, token notifications).
  - Currently, Paste pages don't show tooltips on tokens.
  - `buildApplicationPage()` stopped respecting `pageObjects` (which controls whether "this page has been updated" is shown). Restore that.
  - Make `pageObjects` imply "clear notifications on this stuff".

Test Plan: Viewed a tokened Paste. Verified it cleared the notification and hovering over a token showed a tip.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3218

Differential Revision: https://secure.phabricator.com/D5971
2013-05-19 07:51:31 -07:00
Gareth Evans
94e7878a57 Route internal conduit calls if other hosts available
Summary:
Ref T2785

Looks for hosts in `conduit.servers` config and if any exist route any conduit calls through any one of the hosts.

Test Plan:
Make some curl calls to public methods (`conduit.ping`), watch the access log for two requests. Make some calls from the UI that require authentication, watch the access log a bit more.

Also ran the unit tests.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2785

Differential Revision: https://secure.phabricator.com/D5970
2013-05-19 04:16:10 -07:00
epriestley
5cd13c3c65 Remove the last hardcoding from PhabricatorMetaMTAReceivedMail
Summary:
Moves all remaining mail handling into ReplyHandlers.

Farewell, `getPhabricatorToInformation()`! You were a bad method and no one liked you.

Ref T1205.

Test Plan:
  - Used test console to send mail to Revisions, Tasks, Conpherences and Commits (these all actually work).
  - Used test console to send mail to Requests, Macros, Questions and Mocks (these accept the mail but don't do anything with it, but didn't do anything before either).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5953
2013-05-17 10:00:49 -07:00
epriestley
bb0a39a48c Add loadObject() methods to PhabricatorObjectMailReceiver subclasses
Summary: This doesn't do anything, but touches a bunch of files so I split it out to reduce the size of the next diff. Basically, make `MailReceiver` classes responsible for loading their application objects. Ref T1205.

Test Plan: Inspection / next diff / code is not reached.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5941
2013-05-17 03:47:46 -07:00
Bob Trahan
016b62a7ef Diffusion - move some DiffusionRequest queries to occur over Conduit
Summary: Ref T2784. This one was a wee bit complicated. Had to add PhabricatorUser and concept of initFromConduit (or not) to DiffusionRequest.

Test Plan: foreach repo, visited CALLSIGN and clicked a commit and verified they laoded correctly. Hacked code to hit NOT via Conduit and repeated tests to great success.

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T2784

Differential Revision: https://secure.phabricator.com/D5928
2013-05-14 15:32:19 -07:00
epriestley
341079c3cf Move some received mail responsibility to applications
Summary:
Ref T1205. Continuation of D5915.

Currently, `PhabricatorMetaMTAReceivedMail` has //all// the logic for routing mail. In particular:

  - New mail receivers in applications must edit it.
  - Mail receivers don't drop out when applications are uninstalled.

Applications have some logic in subclasses of `PhabricatorMailReplyHandler`, but this class is a bit of a mess. It is also heavily based on the assumption that mail receivers are objects (like revisions), but this is not true in at least two cases today (creating new tasks with `bugs@`, creating a new Conpherence thread) and likely other cases in the future (e.g., revision-by-mail).

Move this logic into a new `PhabricatorMailReceiver` classtree. This is similar to `PhabricatorMailReplyHandler` but a bit cleaner and more general. I plan to heavily reduce the responsibilities of `PhabricatorMailReplyHandler` or possibly eliminate it entirely.

For now, the new classtree doesn't do much of interest. The only behavioral change this diff causes is that Phabricator will now reject mail to an application when that application is uninstalled.

I also moved all the `ReplyHandler` classes into `mail/` directories in their respective applications.

Test Plan: Unit tests, used receive test to route mail to various objects.

Reviewers: btrahan

Reviewed By: btrahan

CC: Afaque_Hussain, edward, aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5922
2013-05-14 10:57:41 -07:00
epriestley
7c502e3e4e Fix performance issue with large changesets in Differential
Summary:
Fixes T3151. Javelin treats a behavior without parameters as a global behavior and invokes it only once no matter how many times it is initialized (this is necessarily correct for any reasonable behavior, as the inputs do not vary). A recent patch changed `differential-dropdown-menus` from a zero-argument global behavior to an implicitly nonzero-argument behavior by adding `pht`.

Currently, we initialize the behavior next to dropdown menu creation, so this resulted in `O(N^2)` initializations of the menus. For large diffs, this locks browsers. Instead, initialize outside of the dropdown loop so we ginitialize each menu just once.

Test Plan: Viewed a 2,000 file diff without browser lock.

Reviewers: wez, vrana, btrahan

Reviewed By: wez

CC: aran

Maniphest Tasks: T3151

Differential Revision: https://secure.phabricator.com/D5885
2013-05-10 05:39:44 -07:00
Chad Little
13464cc568 Clean up inline edit ui
Summary: Fixes the button spacing issue (doesn't seem related to forms?) and moves fonts and sizes over to Helvetica.

Test Plan: Submit many inline comments.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5882
2013-05-09 16:09:29 -07:00
Chad Little
43ff24b0f3 Update form styles, implement in many places
Summary:
This creates a common form look and feel across the site. I spent a bit of time working out a number of kinks in our various renderings. Some things:

- Font Styles are correctly applied for form elements now.
- Everything lines up!
- Selects are larger, easier to read, interact.
- Inputs have been squared.
- Consistant CSS applied glow (try it!)
- Improved Mobile Responsiveness
- CSS applied to all form elements, not just Aphront
- Many other minor tweaks.

I tried to hit as many high profile forms as possible in an effort to increase consistency. Stopped for now and will follow up after this lands. I know Evan is not a super fan of the glow, but after working with it for a week, it's way cleaner and responsive than the OS controls. Give it a try.

Test Plan: Tested many applications, forms, mobile and tablet.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5860
2013-05-07 14:07:06 -07:00
epriestley
30b15e094c Load commits affected by revert language in Diffusion message parser
Summary:
Ref T1751. This still doesn't do anything very interesting, but loads the acutal Commit objects that a commit message claims to revert.

The only tricky thing here is that we need to interpret "reverts rnnn" or "reverts nnn" in an SVN repository as "reverts rXnnn", where "X" is the current repository. This adds a method to do allow `DiffusionCommitQuery` to do that.

Test Plan:
Used `reparse.php --message` to reparse several commits with revert language and verify they loaded the correct affected commits.

In an SVN repository, created a commit with ambiguous revert language ("reverts n", "reverts rn", "reverts n, n") and verified it identified the affected commits correctly despite ambiguity.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1751

Differential Revision: https://secure.phabricator.com/D5842
2013-05-06 18:05:33 -07:00
deedydas
6eb82ebfd4 xDiffs and Revisions Generating
Summary: Ref T2903

Test Plan: Revisions and Diffs are visually being generated

Reviewers: epriestley, xask.linus

Reviewed By: epriestley

CC: AnhNhan, aran, Korvin

Maniphest Tasks: T2903

Differential Revision: https://secure.phabricator.com/D5835
2013-05-06 14:11:37 -07:00
epriestley
2b2641192a Parse VCS revert langauge in commit messages
Summary:
Ref T1751. When commit messages include language like "reverts X", parse it. This change doesn't do anything with the commits yet.

I attempted to cover all "natural" VCS messages and all reasonable human variations of these messages.

Test Plan: Added unit tests. Added `var_dump()` and used `scripts/repository/reparse.php --message X` to reparse some commit messages, with expected results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1751

Differential Revision: https://secure.phabricator.com/D5840
2013-05-06 12:33:37 -07:00
Mohamed Fawzy
89747c0ea4 Return the changeset ID as part of the changeset dict
Summary:
getDiffDict method returns the JSON for the changesets in a diff
The JSON descriping a changeset didn't containt the changeset ID
in some scenarios, knowing the changeset ID is really useful for the
clients.(i.e. when you want to open a standalone view of the changeset,
    you need to know its ID)

Test Plan: existing

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5816
2013-05-03 08:13:12 -07:00
deedydas
1b4dde8027 Completed Paste Generation
Summary: Ref T2903

Test Plan: Pastes being generated with all types of parents, view policies, code, etc.

Reviewers: epriestley, AnhNhan

Reviewed By: epriestley

CC: AnhNhan, aran, Korvin

Maniphest Tasks: T2903

Differential Revision: https://secure.phabricator.com/D5707
2013-04-29 12:11:01 -07:00
epriestley
614dcbb2d9 Flip strings for 'metamta.differential.unified-comment-context' config
Summary: These should be in (true, false) order, not (false, true) order. This was a transcritpion error in porting the config over.

Test Plan: Edited config from the web, verified the labels matched the underlying values.

Reviewers: tberman, btrahan

Reviewed By: tberman

CC: aran

Differential Revision: https://secure.phabricator.com/D5786
2013-04-26 06:19:07 -07:00
Jakub Vrana
141233565a Fix whitespace around methods 2013-04-23 09:42:35 -07:00
Anh Nhan Nguyen
7924e67287 Making DifferentialListController more mobile-friendly
Summary:
We now have the app menu top-right, so we don't need the side navbar anymore

Refs T2014

Test Plan: Visited list view in narrow / wide browser. Could access everything either directly (tables :P) or the top right menus

Reviewers: epriestley, chad

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2014

Differential Revision: https://secure.phabricator.com/D5728
2013-04-18 12:11:03 -07:00
Jakub Vrana
ed02823eaf Condense renames in Differential ToC
Summary: It's currently quite confusing.

Test Plan:
  lang=diff
  diff --git a/bin/config b/bin/x
  similarity index 100%
  rename from bin/config
  rename to bin/x
  diff --git a/bin/aphlict b/bin/y/aphlict
  similarity index 100%
  rename from bin/aphlict
  rename to bin/y/aphlict
  diff --git a/bin/diviner b/scripts/z
  similarity index 100%
  rename from bin/diviner
  rename to scripts/z

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, AnhNhan

Differential Revision: https://secure.phabricator.com/D5708
2013-04-16 11:02:04 -07:00
Anh Nhan Nguyen
bd350abbe9 Probably added app menu for mobile
Summary:
Messy POC. I hope I did this one right :P

Refs T2014

Test Plan: Switched browser forcefully into `device-phone`, saw filters in the menu top right. Browsed around Differential in desktop mode, nothing was broken, I think.

Reviewers: epriestley, chad, btrahan

CC: aran, Korvin

Maniphest Tasks: T2014

Differential Revision: https://secure.phabricator.com/D5693
2013-04-15 06:52:29 -07:00
Chad Little
57ad790de3 Hovercard tweaks
Summary: Tightens up spacing, remove some of the borders, add alpha channel, make them all blue (sorry, red green and yellow are for 'status'). If we want to do more colors just for hovercards, I have a brown and a black in the mock, but would like to try just blue for now.

Test Plan: UIExamples, Tasks, People, Diffs, and Pastes.

Reviewers: epriestley, AnhNhan, btrahan

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5609
2013-04-06 21:16:55 -07:00
Jakub Vrana
3231df7625 Deprecate 'maniphest.enabled' and 'phriction.enabled'
Summary:
Also join concepts of installed and enabled applications.
Also respect uninstalled Maniphest where disabled Maniphest was checked.

Test Plan:
Visited T1, D1.
Uninstalled Maniphest then visited T1, D1.
Disabled Maniphest then visited T1.
Visited /config/edit/maniphest.enabled/.

Reviewers: epriestley, Afaque_Hussain, edward

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5602
2013-04-06 11:39:59 -07:00
Anh Nhan Nguyen
b951a38a07 Adding hovercard event listeners for Users, Revisions, Conpherence and Commits
Summary:
Refs T1048

Adding Differential Hovercard EventListener

Adding People Hovercard EventListener

Adding basic Diffusion hovercard

Adding Conpherence Hovercard EventListener

Test Plan:
Used in a combo with working hovercards. So beautiful.

Also visited test page. Works alright.

awesometown

Reviewers: epriestley, chad, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T1048

Differential Revision: https://secure.phabricator.com/D5576
2013-04-05 17:01:54 -07:00
Anh Nhan Nguyen
843d0bdcde Introduce some convenience methods for handle rendering
Summary:
So I don't have to copy/paste everything again.

Used them at places I could find with my limited `grep` skills.

Test Plan: Visited hovercards, revision and tasks. No crashes.

Reviewers: epriestley, btrahan, chad

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5592
2013-04-05 17:01:44 -07:00
Anh Nhan Nguyen
d3020af97b Uniformized handle data
Summary:
This sets more reasonable values for the object handle fields imo. It's not like I ever want to find out what letter to use and then do `substr($handle->getType(), 0, 1).$handle->getID()` to get `D1` each time I use handles.

Name:

- D1
- T1
- M1
- P1
- etc.

Fullname:

- D1: Something
- T1: Something
- etc.

In addition, this helps me to reasonable prefill Hovercards in case there is no application-specific event listener.

Also deletes `title` and `alternateID` completely. They deserved that.

Test Plan:
Visited places, nothing broke (We only ever used `$handle->getName()` for users and commits).

Tested mail reply handler. Did not test the other way around, but should be fine.

Hovercards broken until D5572 (would love to induce a cyclic dependency)

Reviewers: epriestley, chad, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5571
2013-04-04 13:10:19 -07:00
Anh Nhan Nguyen
4c6ab5060e Streamline tag rendering for Differential and Maniphest
Summary:
Well, I'm just putting it into the DAO classes, or am I doing something wrong?

Will be used by future event listeners

Test Plan: Visited some tasks and revisions. Look fine.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5542
2013-04-03 08:28:18 -07:00
Jakub Vrana
0de66e9c75 Hide "Maniphest Task:" in commit template
Summary:
We prefer parsing freetext fields.
We should probably wait for {T2743}.

Test Plan:
  $ arc --conduit-uri=http://phabricator.local/ diff

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5349
2013-03-30 12:15:07 -07:00
James Rhodes
e555b9025f Implemented Phrequent time tracking functionality.
Summary:
This differential implements Phrequent's time tracking
functionality for users and hooks it up to Maniphest.  It
also includes a basic "Time Tracked" list for the Phrequent
application, where users can review what they've spent time
working on.

Test Plan:
Apply the patch and track some things in Maniphest.  They
should appear in the "Time Tracked" view of Phrequent.

There is also a `phrequent.show-prompt` option which toggles
whether to display a prompt when tracking time.  I'm unsure
of whether the prompt is useful or is more likely to cause
people to click "Track Time", go off and do the task and then
come back to the prompt still waiting for them to confirm.  A
potential solution to the "accidentally clicking the button
and recording 2 seconds of time" might be to show a prompt
on stop if the total time is under 10 seconds, asking whether
the user wants to keep or discard the tracked time.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2857

Differential Revision: https://secure.phabricator.com/D5479
2013-03-30 09:32:47 -07:00
Anh Nhan Nguyen
9a6c912ef6 Modernizing Flag Application
Summary:
Fixes a weird string in the flag dialog

Migrated to ObjectItemView (Cards)

Removed filters from side nav (unnecessary)

Go away, go away, panel. Nobody will miss you.

Adding sorting/ordering to Flag (separation like in Maniphest ordering still remaining)

Hacking our way in DifferentialRevisionListController thanks to panels (who added `->setFlush()` btw? It's awesome!)

Test Plan: I would not dare to stand here if it did not work.

Reviewers: epriestley, chad, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5449
2013-03-26 14:10:12 -07:00
epriestley
ebdf5d12de Remove line-related stats from Differential
Summary:
Ref T2825. Line stats are misleading when multiple commits are associated with a revision. It's better to have no data than misleading/bad data.

We can compute them accurately against commits via Fact/ETL at some point far in the future (see T1562 / T1135).

Test Plan: {F37519}

Reviewers: ptarjan

Reviewed By: ptarjan

CC: aran

Maniphest Tasks: T2825

Differential Revision: https://secure.phabricator.com/D5432
2013-03-26 13:59:02 -07:00
Chad Little
8a0fccf97a Mobile Crumbs.
Summary: Not for full review. This makes crumbs appear consistently in mobile. It helps give a quick link to the apps home, the page title currently on, and action icons for the object. It will take additional clean-up to make this consistent across apps. Passing for early review from a UEX perspective. I actually really like it and think onces it's everywhere, helps mobile feel complete.

Test Plan: Testing in iOS and Simulator.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2796

Differential Revision: https://secure.phabricator.com/D5446
2013-03-26 13:15:15 -07:00
Afaque Hussain
d43bcdc1ee Adding pro tips to Differential commit message
Summary: Randomly displaying tips about different features of arc.

Test Plan: Will test by running arc diff and seeing if the tips appear in text-based UI of arc

Reviewers: epriestley, AnhNhan

Reviewed By: AnhNhan

CC: aran, Korvin, AnhNhan

Differential Revision: https://secure.phabricator.com/D5351
2013-03-19 09:29:25 -07:00
Anh Nhan Nguyen
038869c056 Fixing D5371
Summary:
Fixes T2765

Wrong brace placement D:

Test Plan: `/api/differential.getcommitmessage` with some set field

Reviewers: epriestley

CC: aran, Korvin

Maniphest Tasks: T2765

Differential Revision: https://secure.phabricator.com/D5376
2013-03-19 06:03:13 -07:00
Anh Nhan Nguyen
1a92976c14 Deduplicating duplicated field values for Commit Messages / Differential
Summary: Especially when doing 'arc diff' containing multiple commits which have pre-filled template fields. Names would pop up multiple times

Test Plan:
{F36314}

Local `arc diff` with some uber-branch also agrees with me

Reviewers: epriestley, vrana

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5371
2013-03-18 10:37:40 -04:00
Afaque Hussain
05c5c43159 Yell at developers if they don't replace the default revision title with thier new revision title.
Summary: Same as the title

Test Plan: Will test locally by running arc diff

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, AnhNhan, starruler

Maniphest Tasks: T2663

Differential Revision: https://secure.phabricator.com/D5297
2013-03-13 09:34:29 -07:00
epriestley
855e085c6f Uninstall Conduit calls when uninstalling applications
Summary: Fixes T2698. When applications are installed, their Conduit calls should drop out. This will also let us land Releeph without exposing Conduit calls.

Test Plan:
  - Viewed Conduit console; uninstalled some applications and verified their calls dropped out.
  - Tried to make an uninstalled call; got an appropriate error.

Reviewers: edward, btrahan

Reviewed By: edward

CC: aran

Maniphest Tasks: T2698

Differential Revision: https://secure.phabricator.com/D5302
2013-03-13 07:09:05 -07:00
Chad Little
df0c3df3cc Modernize Maniphest
Summary:
A few things
- pht Maniphest where I could
- implement dust background
- optimize pages for mobile
- adds aphront-two-column-layout
- reworks maniphest page with two column layout
- tweaks task table for mobile, though we should move to object-list-view

Stopping here as I want to get feedback in. Super excited about mobile and the new tasks views. Only sort of excited about the sidebar filters, they need more UI work, but we should talk about that.

Test Plan: Test Maniphest, Differential, and Homepage views. Sort tasks, make reports

Reviewers: epriestley, btrahan

Reviewed By: btrahan

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5314
2013-03-12 23:30:03 -07:00
Jakub Vrana
47f6aecb72 Parse "Depends on" in freeform Differential fields
Summary:
I considered also parsing "Depends On:" field in the commit message but it's more involved and I also prefer having this information in the summary where it's more visible.
I also didn't want the field displayed by default so user would have to write "Depends On:" anyway.

Test Plan: Used "Depends on D1" in summary, saw it in dependencies.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5300
2013-03-10 11:00:02 -07:00
epriestley
4c914a5c49 Remove all calls to renderSingleView() and deprecate it
Summary: After D5305, this method does nothing since we automatically figure out what we need to do.

Test Plan:
- Viewed a page with the main menu on it (MainMenuView).
- Viewed a revision with transactions on it (TransactionView).
- Viewed timeline UIExample (TimelineView, TimelineEventView).
- Viewed a revision (PropertyListView).
- Viewed a profile (ProfileHeaderView).
- Viewed Pholio list (PinboardView, PinboardItemView).
- Viewed Config (ObjectItemView, ObjectItemListView).
- Viewed Home (MenuView).
- Viewed a revision (HeaderView, CrumbsView, ActionListView).
- Viewed a revision with an inline comment (anchorview).
- Viewed a Phriction diff page (AphrontCrumbsView).
  - Filed T2721 to get rid of this.
- Looked at Pholio and made inlines and comments (mockimages, pholioinlinecomment/save/edit).
- Looked at conpherences.
- Browsed around.

Reviewers: chad, vrana

Reviewed By: chad

CC: edward, aran

Differential Revision: https://secure.phabricator.com/D5307
2013-03-09 13:52:41 -08:00
epriestley
5e41ead7c7 Have AphrontView implement PhutilSafeHTMLProducerInterface
Summary: Allows views to work like tags.

Test Plan: Implemented a few completely arbitrary render() / singleView simplifications. I just picked some that were easy to test. I'll do a more thorough pass on this in a followup; these calls don't really hurt anything.

Reviewers: chad, vrana

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D5306
2013-03-09 13:52:21 -08:00
epriestley
c91253c691 Remove "MetaMTA Transcripts" and "Herald Transcripts" links from Differential
Summary: These actions are dumb, and not smart, and no one likes them.

Test Plan: Looked at a revision and saw fewer actions; higher average action quality.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D5304
2013-03-09 09:22:10 -08:00
Afaque Hussain
7acaeee844 Augmented DifferentailDependsOnFieldSpecification to expose its data over conduit
Summary: Implmented conduit methods from base class in DifferentialDependsOnFieldSpecification

Test Plan: Created two test diffs, made one of them dependent on another. Then checked the dependency over conduit interface for differential.query. Dependency is shown but in terms of PHIDs

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5238
2013-03-05 12:28:09 -08:00
Afaque Hussain
a521790f13 Ported loadAuxiliaryFields method to differential.query conduit method
Summary: Ported the the loadAuxiliaryfields method from differential.getrevision method to modern conduit differential.query method

Test Plan: Created a test diff in differential to see if nothing has been broken

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5230
2013-03-05 05:58:04 -08:00
epriestley
8ae718c2aa Require a viewer for Remarkup rendering
Summary:
Provide a viewer to all remarkup engines.

This fixes commit summaries in Diffusion, which were failing to link because they didn't have a user and thus couldn't see/load `D123`, e.g.

Test Plan: Grepped for engine creation.

Reviewers: vrana

Reviewed By: vrana

CC: aran, edward

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D5152
2013-03-04 12:33:05 -08:00
vrana
ab5e019b3d Pass user to DifferentialRevisionDetailRenderer
Summary: I know that this code would be replaced by something else but until then...

Test Plan: Used it in our renderer.

Reviewers: epriestley, edward

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5165
2013-03-04 11:32:02 -08:00
epriestley
31cc78cc35 Fix a missing setActor() on DifferentialNewDiffMail
Auditors: vrana
2013-02-28 17:23:23 -08:00
epriestley
0a069cb55a Require a viewer to load handles
Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.

There are a few notable cases here:

  - I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
  - I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
  - I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.

Test Plan:
  - Grepped for all PhabricatorObjectHandleData references.
  - Gave them viewers.

Reviewers: vrana

Reviewed By: vrana

CC: aran, edward

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D5151
2013-02-28 17:15:09 -08:00
epriestley
5754bec293 Gratuitous hacks until D5151 lands
Auditors: vrana
2013-02-28 08:05:43 -08:00
epriestley
ab5ab5a7e5 Unbreak commenting on Reivsions with commits attached until D5151 can land
Auditors: vrana
2013-02-28 08:03:38 -08:00
epriestley
b5da7b3723 Missing change from D5120 which I incorrectly discarded in a merge.
Auditors: vrana
2013-02-27 07:19:57 -08:00
vrana
40d453d6aa Limit number of affected files in Differential e-mails
Summary: This triggers mainly for SVN branch copy.

Test Plan: Ran this code separately.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5126
2013-02-26 15:00:24 -08:00
epriestley
5e11eb7f72 Prepare for hovercards
Summary:
  - Unify all the reference/embed Remarkup rules for Differential, Maniphest, Paste and Ponder.
  - Add rules for Pholio.
  - Does not yet unify Diffusion or Files (both are a bit more involved).
  - Prepare for hovercards.

Test Plan: {F33894}

Reviewers: chad, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D5120
2013-02-26 14:59:31 -08:00
epriestley
fe500f4268 Pre-prepare for hovercards
Summary:
D5120 and followups refactor and generalize object references in Remarkup -- notably, they move remarkup rules from a central location to the implementing applications.

Preserve blame by doing moves/renames only first. This change moves application remarkup rules into those applications, and renames the ones D5120 modifies.

Test Plan: Typed some preview text into a textarea, got a valid Remarkup render.

Reviewers: vrana, chad

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D5123
2013-02-26 14:57:41 -08:00
Nick Harper
894cd13a41 Get rid of null characters
Summary:
We had some null bytes appearing in strings from unit test results, which
caused the PhutilRemarkupEngine to fail at properly generating html for it.
Specifically, the string would get cut off at the null byte, and the closing
</p> tag would never get added. The lack of this tag caused the dom for the
rest of the page to end up inside a hidden td in the unit test results table.

This is a horrible hack of a solution for this - it would be better to fix
PhutilRemarkupEngine (and in the future, strip out null bytes in input in
strings).

Test Plan: load a differential revision and see content after the unittest results

Reviewers: epriestley, vrana

Reviewed By: vrana

CC: aran, Korvin, rm

Differential Revision: https://secure.phabricator.com/D5065
2013-02-22 10:07:09 -08:00
vrana
6404fa5c0e Don't display 0 in launch view
Summary: This includes assigned tasks in the Maniphest number.

Test Plan: Looked at it.

Reviewers: chad, epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D5067
2013-02-22 05:01:30 -08:00
vrana
c29fe2deb6 Display application status in tip for needs attention number
Summary:
Also splits blocking and active revisions.

This could display 0 with non-empty tip over it.
It's intentional meaning that 0 objects need your attention but there is still some work to do.

Test Plan: Hovered over number.

Reviewers: epriestley, chad

Reviewed By: chad

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5049
2013-02-21 14:57:24 -08:00
vrana
3c989590bf Remove object name from header
Summary: It's displayed right above it in the breadcrumbs including a link.

Test Plan: Looked at the pages.

Reviewers: chad, epriestley

Reviewed By: epriestley

CC: aran, epriestley, s.o.butler

Differential Revision: https://secure.phabricator.com/D5045
2013-02-21 11:00:22 -08:00
vrana
fbd41e4219 Move revisions without reviewers to author's queue
Summary:
If the revision doesn't have reviewers then it's really not waiting on someone else and the author must take an action.
An improvement would be to check if the reviewers are not disabled but that would require loading their handles.

Test Plan:
/
/differential/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, s.o.butler

Differential Revision: https://secure.phabricator.com/D5046
2013-02-21 10:13:40 -08:00
vrana
2191e99b49 Delete unused variable 2013-02-19 16:08:04 -08:00
vrana
ee663afdeb Initialize variable in Next Step
Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5020
2013-02-19 16:07:09 -08:00
epriestley
17cabea1bc I am not good at computer
Auditors: vrana
2013-02-19 15:25:44 -08:00
epriestley
db2c32acdf Don't fatal for diffs with no VCS
Auditors: vrana
2013-02-19 15:19:28 -08:00
epriestley
0d38bca11f Fix function/method declaration lint in Phabricator. Automatic patches.
Auditors: vrana
2013-02-19 13:47:03 -08:00
epriestley
a22bea2a74 Apply lint rules to Phabricator
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such

Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D5002
2013-02-19 13:33:10 -08:00
vrana
63f35ee94f Suggest branch name in Next Step
Summary: Is this correct for Mercurial?

Test Plan: Saw it.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4957
2013-02-19 12:58:16 -08:00
vrana
9a3d0f71a3 Decrease commit identifier display length
Summary: We are running out of horizontal space, this should help a bit.

Test Plan: Homepage, revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4518
2013-02-19 12:43:57 -08:00
epriestley
a5f031835c Notify users when an object they created gets awarded a token
Summary:
  - Publish feed/notification.
  - I think this is too lightweight for an email?
  - We don't tell them which token right now. Laziness? Or intentional aura of mystery?!
  - For tasks, notify both author and current owner.
  - Fixes T2562.

Test Plan: {F33187}

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T2562

Differential Revision: https://secure.phabricator.com/D5007
2013-02-18 17:44:45 -08:00
epriestley
2231e5200a Implement basic transaction detail blocks
Summary:
Some transactions (like editing configuration values, task descriptions, or Conpherence images) can't be simply explained and need an additional larger element to show them fully (like a text diff).

Support change details like this in ApplicationTransactions. Implements the element in Config, so you can see changes.

Test Plan: {F32974}

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T2213

Differential Revision: https://secure.phabricator.com/D4984
2013-02-17 06:37:02 -08:00
epriestley
49c40d209d Tokens v1
Summary:
Features!

  - Giving tokens.
  - Taking tokens back.
  - Not giving tokens.

Test Plan: See screenshots.

Reviewers: chad, vrana

Reviewed By: chad

CC: aran, btrahan

Maniphest Tasks: T2541

Differential Revision: https://secure.phabricator.com/D4964
2013-02-15 07:47:14 -08:00
epriestley
4f42d85c1c Use a serialize()-based cache instead of a JSON-based cache for changesets
Summary: These are full of PhutilSafeHTML objects now, which are destroyed by JSON serialization.

Test Plan: Dropped cache, then reloaded pages.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4942
2013-02-13 14:45:57 -08:00
epriestley
262abd39bc Fix undefined variable "none"
Summary: Assuming this is right?

Test Plan: No more exception error when viewing a revision.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4937
2013-02-13 13:40:46 -08:00
epriestley
4bd2ad9270 Merge branch 'master' into phutil_tag
Auditors: vrana
2013-02-13 12:42:57 -08:00
vrana
4eb84149c2 Convert everything to safe HTML
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.

Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.

Reviewers: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4927
2013-02-13 12:35:40 -08:00
vrana
718d22d607 Convert Remarkup to safe HTML
Test Plan: None.

Reviewers: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4919
2013-02-13 12:34:49 -08:00
vrana
5a9e834658 Dont display empty other locations of lint errors 2013-02-12 14:01:27 -08:00
vrana
80fb84bd94 Convert PhabricatorTransactionView to safe HTML
Test Plan: Looked at revision detail with comments.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4915
2013-02-11 19:01:20 -08:00
vrana
868ca71451 Fix some HTML problems
Summary: I'm too lazy to attaching them for diffs where they were introduced.

Test Plan:
/
/D1, wrote comment with code snippet
DarkConsole
commit detail, wrote comment
task detail, wrote comment

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4911
2013-02-11 18:18:26 -08:00
vrana
8c71815028 Merge renderChildren() and renderHTMLChildren()
Summary: `renderChildren()` now returns array which isn't ideal but I prefer it to having two methods.

Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4910
2013-02-11 18:18:23 -08:00
vrana
ae4e5807d6 Merge renderSingleView() and renderHTMLView()
Summary: They are same because render() returns safe HTML and raw strings are automatically escaped.

Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4909
2013-02-11 18:18:21 -08:00
vrana
c9ab1fe505 Return safe HTML from all render()
Summary:
This is pretty brutal and it adds some `phutil_safe_html()`.
But it is a big step in the right direction.

Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4905
2013-02-11 18:18:18 -08:00
vrana
37b98450a5 Replace array_interleave() by phutil_implode_html()
Summary:
I like this abstraction better.
Result of `phutil_implode_html()` may be also used as a param of `hsprintf()`.

Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4904
2013-02-11 15:27:43 -08:00
vrana
a22ef4e9b4 Kill most of phutil_escape_html()
Summary:
This resolves lots of double escaping.
We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped.
Also `pht()` auto escapes if it gets `PhutilSafeHTML`.

Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4889
2013-02-11 15:27:38 -08:00
vrana
b72e7969a1 Move Pygments handling to libphutil
Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4875
2013-02-09 15:19:55 -08:00
vrana
9b8da73765 Convert AphrontTableView to safe HTML
Summary:
Lots of killed `phutil_escape_html()`.

Done by searching for `AphrontTableView` and then `$rows` (usually) backwards.

Test Plan:
Looked at homepage.

  echo id(new AphrontTableView(array(array('<'))))->render();

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4884
2013-02-09 15:11:38 -08:00
vrana
58b6e2cac6 Convert AphrontDialogView to safe HTML
Summary:
Done by searching for `AphrontDialogView` and then `appendChild()`.

Also added some `pht()`.

Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4882
2013-02-09 15:11:35 -08:00
epriestley
7ec8e885e1 Merge branch 'master' into phutil_tag
(Final final sync.)
2013-02-08 17:29:32 -08:00
vrana
d817dfa8fc Convert some phutil_escape_html() to hsprintf()
Summary: Found by `sgrep_php -e '"...".phutil_escape_html(...)'`.

Test Plan:
/
/D1
/uiexample/
/countdown/1/
/herald/transcript/1/all/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4869
2013-02-08 15:59:02 -08:00