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

5727 commits

Author SHA1 Message Date
Chad Little
4135c7de37 Clean up FontAwesome / Timeline CSS
Summary: Normalize FA CSS, expand timeline icons to 14px, fix mobile browsers, give attribution in uiexamples

Test Plan: tested iOS simulator, chrome, ie

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8845
2014-04-22 18:29:14 -07:00
Chad Little
094c79d6e2 Make reject, accept outline icons
Summary: These are a little easier on the eyes.

Test Plan:
Reject an epriestley diff.

{F146851}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8841
2014-04-22 14:55:45 -07:00
epriestley
a88f09469d Adjust reviewer transaction icon in Differential
Summary:
Ref T4866. I did a fancy version of this but it looks pretty bad/confusing so here's a simple version.

Fancy-but-whack version:

{F146847}

Test Plan: This version is like that, but just always uses `fa-user`.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4866

Differential Revision: https://secure.phabricator.com/D8840
2014-04-22 14:32:45 -07:00
Chad Little
9d3f8117e7 More resilient timeline icon layout
Summary: center aligns the icons in the fill area, removes some of the positioning jank. Also set new icons for maniphest custom.

Test Plan: test desktop and mobile layouts, tested thin pins for proper centering.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4866

Differential Revision: https://secure.phabricator.com/D8839
2014-04-22 14:24:36 -07:00
Chad Little
11fd6afeb1 Move Timeline icons to Fonts
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.

Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8827
2014-04-22 08:25:54 -07:00
Chad Little
31b1cc6cd8 Remove Glyphicons Fonts
Summary: For the time being, no need to have these in the repository.

Test Plan: Reload UIExamples, only see FontAwesome

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8835
2014-04-21 21:09:25 -07:00
epriestley
3d12bbb17c Minor, improve notification resiliance
Summary: The token transactions can publish empty transaction feed stories.
Stop them from doing that, and make notifications fail more quietly.

Auditors: btrahan
2014-04-21 16:28:59 -07:00
epriestley
fcf5149b36 Clean up numerous rough edges in Mail configuration
Summary:
  - Support file attachments in Mailgun, after D8831.
  - Fix `bin/mail send-test --attach ...` flag.
  - Make `bin/mail send-test` route mail through the daemons.
  - Remove the `workerTaskID` on MetaMTAMail, which is only used (needlessly) by `bin/mail resend` and creates a huge mess elsewhere.
  - Currently, when mail fails, the daemon exits with a very generic and useless message. Instead, make `sendNow()` throw when it fails, so the real reason is surfaced. This is OK now because mail is always sent via the daemons.
  - Now that Mailgun supports attachments, document it.
  - Update a bunch of mail docs.

Test Plan:
  - Sent mail.
  - Sent mail with attachments.
  - Read documentation.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8832
2014-04-21 15:45:29 -07:00
Bob Trahan
c00733a292 Project - add editor-level support for detecting name conflicts
Summary: Fixes T4833. I wish there was an elegant way to catch this exception but I think the stack is written such that we really should just do this one-off query here...

Test Plan: from the "create project" link under "edit task" I received a more detailed exception than the report in T4833 post patch. I also tested editing an existing project - yay - and editing an existing project to some other existing project's name - got a nice error dialogue.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4833

Differential Revision: https://secure.phabricator.com/D8834
2014-04-21 15:44:35 -07:00
epriestley
5cbdda413c Paradigms, paradigms, paradigms
Summary: Fixes T4693.

Test Plan: {F146407}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4693

Differential Revision: https://secure.phabricator.com/D8829
2014-04-21 15:33:59 -07:00
epriestley
0cfc5aa0aa Support "public" policy in Tokens
Summary: Ref T4830. Allows logged-out users to browse tokens, policies permitting.

Test Plan: Browsed tokens as a logged-out user.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4830

Differential Revision: https://secure.phabricator.com/D8828
2014-04-21 15:33:07 -07:00
epriestley
27d426e3fe Allow Conduit console to be browsed by logged-out users
Summary:
Ref T4830. A few methods, like `conduit.ping`, are callable without authentication, so this even has some use cases. Also:

  - Make some Differential stuff a little more consistent.
  - Use slightly more modern rendering.
  - Deprecate the status-oriented `user` calls; these will be replaced by Calendar methods.

Test Plan: Browsed console as logged out / logged in users.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4830

Differential Revision: https://secure.phabricator.com/D8826
2014-04-21 15:32:48 -07:00
epriestley
4143ecf015 Minor updates to UIExamples
Summary:
Ref T4830.

  - If the application policy is public, allow logged-out users to browse examples.
  - Use standard elements instead of custom ones.

Test Plan: Browsed UIExamples.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4830

Differential Revision: https://secure.phabricator.com/D8825
2014-04-21 15:32:03 -07:00
epriestley
28696d08ac Remove indirect loads of Differential revisions from Releeph requests
Summary:
Ref T3551. Currently, there are many layers of indirection between pull requests and revisions. After D8822, revisions and other types of requested objects are recorded directly on the request. This allows us to simplify data access and querying.

A lot of stuff here is doing `instanceof` checks to keep APIs stable, but most of those can go away in the long run.

Test Plan:
  - Browsed requests.
  - Verified revision-dependent fields (like "Revision", "Size", "Churn") still render correctly.
  - Called `releeph.queryrequests`.
  - Called `releephwork.nextrequest`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

Differential Revision: https://secure.phabricator.com/D8824
2014-04-20 11:55:47 -07:00
epriestley
65913162e7 Allow discovered but unparsed commits to be requested in Releeph
Summary:
Ref T3662. Releeph blocks users from requsting unparsed commits, but there's no real technical reason for this.

The `releephwork.getorigcommitmessage` method assumes data exists, but should be replaced with `diffusion.querycommits` anyway.

Test Plan: Ran `diffusion.querycommits`. Requested a commit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3662

Differential Revision: https://secure.phabricator.com/D8823
2014-04-20 11:55:29 -07:00
epriestley
1a3ac09975 Add "requestedObjectPHID" to ReleephRequest
Summary:
Ref T3551. Currently, ReleephRequests don't have a direct concept of the //object// being requested. You can request `D123`, but that is just a convenient way to write `rXyyyy`.

When the UI wants to display information about a revision, it deduces it by examining the commit.

This is primarily an attack on T3551, so we don't need to load <commit -> edge -> revision> (in an ad-hoc way) to get revisions. Instead, when you request a revision we keep track of it and can load it directly later.

Later, this will let us do more things: for example, if you request a branch, we can automatically update the commits (as GitHub does), etc. (Repository branches will need PHIDs first, of course.)

This adds and populates the column but doesn't use it yet. The second part of the migration could safely be run while Phabricator is up, although even for Facebook this table is probably quite small.

Test Plan:
  - Ran migration.
  - Verified existing requests associated sensibly.
  - Created a new commit request.
  - Created a new revision request.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

Differential Revision: https://secure.phabricator.com/D8822
2014-04-20 11:55:18 -07:00
epriestley
a588955bf7 Remove loadPhabricatorRepository from ReleephProject
Summary: Ref T3551. Repository is guaranteed if a product is loaded with modern mechanisms.

Test Plan:
  - Edited a request.
  - Called `releephwork.getbranchcommitmessage`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

Differential Revision: https://secure.phabricator.com/D8821
2014-04-20 11:55:09 -07:00
epriestley
d75d0acba5 Remove loadReleephBranch and loadReleephProject from ReleephRequest
Summary: Ref T3551. Ref T3549. Mostly unnecessary with modern calls.

Test Plan:
- Called `releeph.queryrequests`.
- Called `releeph.request`.
- Called `releephwork.getbranchcommitmessage`.
- Called `releephwork.getcommitmessage`.
- Called `releephwork.nextrequest`.
- Viewed and edited branches and requests.
- Made a comment on a request.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549, T3551

Differential Revision: https://secure.phabricator.com/D8820
2014-04-20 11:54:58 -07:00
epriestley
a4f975ef3e Remove some ad-hoc handle loads from Releeph
Summary:
Ref T3551. Releeph does a bunch of old-school on-object data loading; start cleaning that up.

This doesn't change anything, just makes the code more modern/consistent.

Test Plan: Edited a request; called `releephwork.nextrequest`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

Differential Revision: https://secure.phabricator.com/D8819
2014-04-20 11:54:50 -07:00
epriestley
230fce735e Move /project/ controllers to /product/
Summary: Ref T3549. Move only, no extra changes.

Test Plan: Loaded application.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549

Differential Revision: https://secure.phabricator.com/D8817
2014-04-20 11:54:37 -07:00
epriestley
6e6ad2cfcf Add modern releeph.queryproducts and releeph.querybranches
Summary:
Ref T3662. Ref T3549. These methods are pretty conservative for now, but get the structure in place.

Also do a bunch more project -> product stuff.

Test Plan: Made calls to both methods, browsed around the UI a fair amount.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549, T3662

Differential Revision: https://secure.phabricator.com/D8816
2014-04-20 11:54:22 -07:00
epriestley
f5cc5c122a Rename ReleephProjectQuery to ReleephProductQuery
Summary: Ref T3549.

Test Plan: grep/lint

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549

Differential Revision: https://secure.phabricator.com/D8815
2014-04-20 11:51:02 -07:00
austinkelleher
2e5065feb5 Update function name to follow naming convention.
See: <http://github.com/facebook/phabricator/pull/575>

Reviewed by: epriestley
2014-04-20 08:37:37 -07:00
Chad Little
ce1dbbec3c Add FontAwesome as a font/icon choice
Summary:
This adds FontAwesome and attempts to make use as icons as consistent as possible. May require additional tweaks once we start using, but in practice this is pretty finished.

 - Adds FontAwesome
 - Adds additional transforms (rotates, spins)
 - Adds additional colors
 - Better scopes halflings and fontawesome
 - Shares CSS between fonts for consistency

Test Plan:
Tested various browsers back to IE8, mobile.

{F146146}

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8818
2014-04-19 10:00:37 -07:00
epriestley
5570ec5156 Rename Releeph "Project" PHID type to "Product"
Summary: Ref T3549.

Test Plan: `grep`, loaded some pages

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549

Differential Revision: https://secure.phabricator.com/D8811
2014-04-18 17:52:45 -07:00
epriestley
fde4ccf9b2 Use standard handle loading in Releeph
Summary: Ref T3718. Move from unbatched / ad-hoc loading to standard stuff for handles.

Test Plan: Looked at some requests and saw no changes.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3718

Differential Revision: https://secure.phabricator.com/D8810
2014-04-18 17:52:32 -07:00
epriestley
c7f5dc0208 Remove some unused Releeph field methods
Summary: Ref T3718. Remove `bulkLoad()` and header-view rendering wrappers.

Test Plan: Viewed a request, looked the same. `grep`'d for stuff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3718

Differential Revision: https://secure.phabricator.com/D8809
2014-04-18 17:52:22 -07:00
epriestley
71ef48b3e5 When a diff has 12,345 lines, render "12,345 lines" instead of "12 lines"
Summary: This `%d` should be a `%s`, since the `PhutilNumber` value may get formatted according to locale settings.

Test Plan: will make @zeeg

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, zeeg

Differential Revision: https://secure.phabricator.com/D8814
2014-04-18 17:52:11 -07:00
epriestley
9889892e5b Actually squelch Harbormaster "test passed" mail
Summary:
Ref T1049. When Harbormaster tests pass, don't bother sending an email about it.

(I tried to implement this earlier but didn't test it entirely properly, and we needed a little more code.)

Test Plan: Used `bin/harbormaster build` to build some junk, got no email about passes.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8813
2014-04-18 17:51:59 -07:00
epriestley
f1245f4f34 Remove flavor text for action buttons
Summary: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.

Test Plan: Looked at a revision, task, paste, macro, etc.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8812
2014-04-18 17:51:46 -07:00
epriestley
35df988036 Use standard UI elements to render pull requests in Releeph
Summary:
Ref T3718. Ref T3644. Ref T3092. Switches from the Releeph UI elements to standard ones. I'll attach some screenshots.

Also fixes CSRF against the request action endpoint.

Test Plan:
  - Viewed request details.
  - Took actions on a request from detail page.
  - Viewed request list.
  - Took actions on a request from list page.
  - Used keyboard shortcuts to navigate list.
  - Used keyboard shortcuts to take actions.
  - Simulated errors.
  - Viewed on devices.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: grp, FacebookPOC, mattlqx, tala, beng, LegNeato, epriestley

Maniphest Tasks: T3718, T3092, T3644

Differential Revision: https://secure.phabricator.com/D8771
2014-04-18 06:44:45 -07:00
Chad Little
41ea90c686 Add Glyphicons Halflings Font and Examples
Summary: This adds in the Glyphicons Halflings Font/Iconset as an option for PHUIIconView along with a standard set of 10 colors. This will be a replacement for the standard action icon set in upcoming diffs, as well as obviously give us more flexibility, less KB, and less design resource time managing images.

Test Plan: UIExamples, Diviner

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8798
2014-04-17 17:31:23 -07:00
epriestley
95a405da10 Record build success or failure on buildable objects
Summary:
Fixes T4810. When a buildable completes, make an effort to update the corresponding object with a success or failure message. Commits don't support this yet, but revisions do.

{F144614}

Test Plan:
  - Used `bin/harbormaster build` and `bin/harbormaster update` to run a pile of builds.
  - Tried good/bad builds.
  - Sent some normal mail to make sure the mail reentrancy change didn't break stuff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4810

Differential Revision: https://secure.phabricator.com/D8803
2014-04-17 16:04:14 -07:00
epriestley
49bc32f12d Implement PhabricatorApplicationTransactionInterface in Differential
Summary:
Ref T4810. Ultimate goal is to let Harbormaster post a "build passed/failed" transaction. To prepare for that, implement `PhabricatorApplicationTransactionInterface` in Differential.

To allow Harbormaster to take action on //diffs// but have the transactions apply to //revisions//, I added a new method so that objects can redirect transactions to some other object.

Test Plan:
  - Subscribed/unsubscribed/attached/detached from Differential, saw transactions appear properly.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4810

Differential Revision: https://secure.phabricator.com/D8802
2014-04-17 16:03:24 -07:00
epriestley
62973e0f53 Add activeDiffPHID to differential.query
Summary: Ref T4809. This saves us a few round trips to find a Buildable, and generally makes the notion of "active" more explicit (i.e., not just the diff with the largest ID). In the future, we may let you revert to previous diffs, which would make the "largest number" rule not always correct.

Test Plan: Ran `differential.query`, got sensible results.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4809

Differential Revision: https://secure.phabricator.com/D8800
2014-04-17 16:01:27 -07:00
epriestley
0ef599e906 Give Buildables a status, populate it, and return it over Conduit
Summary:
Ref T4809. Currently, buildables have a status field but nothing populates it. Populate it:

  - When builds change state, update the Buildable state.
  - Use the new Buildable state on the web UI.
  - Return the new Buildable state from Conduit.

To make it easier to debug/test this:

  - Provide `bin/harbormaster update Bxxx ...` to force foreground update of a Buildable.

Test Plan:
  - Used `bin/harbormaster update Bxxx --force --trace` to update buildables.
  - Looked at buidlable list, saw statuses reported properly.
  - Used Conduit to read statuses.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4809

Differential Revision: https://secure.phabricator.com/D8799
2014-04-17 16:01:16 -07:00
epriestley
4918773afe Drop nonsense buildStatus field from Buildable
Summary:
Ref T4809. Buildables currently have buildStatus and buildableStatus. Neither are used, and no one knows why we have two.

I'm going to use buildableStatus shortly, but buildStatus is meaningless; burn it.

Test Plan: `grep`, examined similar get/set calls, created a new buildable, ran storage upgrade.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4809

Differential Revision: https://secure.phabricator.com/D8796
2014-04-17 16:01:06 -07:00
epriestley
7c1bcdea16 Add "harbormaster.querybuilds" Conduit API
Summary:
Ref T4809. This one is more straightforward. A couple of tweaks:

  - Remove the WAITING status, since nothing ever sets it and I suspect nothing ever will with the modern way artifacts work (maybe). At a minimum, it's confusing with the new Target status that's also called "WAITING" but means something different.
  - Consolidate 17 copies of these status names into one method.

Test Plan: Ran some queries via Conduit, got reasonable looking results.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4809

Differential Revision: https://secure.phabricator.com/D8795
2014-04-17 16:00:58 -07:00
epriestley
3b0be0961c Add a rough harbormaster.querybuildables Conduit API method
Summary: Ref T4809. I need to sort out some of the "status" stuff we're doing before this is actually useful (there's no sensible "status" value to expose right now) but once that happens `arc` can query this to figure out whether it needs to warn the user about pending/failed builds.

Test Plan: Ran query with various different parameters.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4809

Differential Revision: https://secure.phabricator.com/D8794
2014-04-17 16:00:25 -07:00
epriestley
b5df5af04d Link to Herald transcripts from Herald transactions
Summary: See IRC. Some users are having difficulty figuring out why Herald is taking some actions. Make it easier to get to the transcript.

Test Plan: {F144622}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: dctrwatson, epriestley

Differential Revision: https://secure.phabricator.com/D8804
2014-04-17 15:57:34 -07:00
epriestley
e49251ec98 Improve robustnesss of feed text rendering
Summary:
Couple of minor cleanup things here:

  - Pass handles to ApplicationTransactions when rendering their stories; this happened implicitly before but doesn't now.
  - Add `?text=1` to do ad-hoc rendering of a story in text mode.
  - Make Conduit skip unrenderable stories.
  - Fix/modernize some text in the Commit story.

Test Plan: Rendered text versions of stories via Conduit and `?text=1`.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: zeeg, spicyj, epriestley

Differential Revision: https://secure.phabricator.com/D8793
2014-04-17 15:57:18 -07:00
epriestley
4a6d2e9c97 Allow tasks to yield to other tasks
Summary:
For Harbormaster tasks which want to poll or wait, this lets them say "try again a little later" without having to sleep and hold a queue slot.

This is basically the same as failing, except that we don't increment the failure counter. Instead, we just set the current lease to the correct length and then exit. The task will be retried after the lease expires.

Test Plan: Using both `bin/harbormaster` and `phd debug taskmaster`, ran a lot of waiting tasks through the queue, faking them to either yield or not yield in a controlled manner. The queue responded as expected, yielding tasks appropraitely and retrying them later.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8792
2014-04-16 13:02:12 -07:00
epriestley
afd04731ab Add a "Create build step" transaction to Harbormaster
Summary:
Without this, build steps that have no options (like "wait for previous commits") don't actually save, since the transaction array is empty.

This also generally nice and consistent.

Test Plan: Created a new "wait" step, viewed transaction log.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8791
2014-04-16 13:01:56 -07:00
epriestley
78bf266bde Allow Harbormaster build targets to wait for messages
Summary:
This hooks up all the pieces of the build pipeline so `harbormaster.sendmessage` actually works. Particularly:

  - Candidate build steps (i.e., those which interact with external systems) can now "Wait for Message". This pauses them indefinitely when they complete, until something calls `harbormaster.sendmessage`.
  - After processing a target, we check if we should move it to PASSED or WAITING.
  - Before updating a build, we move WAITING targets with pending messages to either PASSED or FAILED.
  - I added an explicit "Building" state, which doesn't affect workflows but communicates more information to human users.

A big part of this is avoiding races. I believe we get the correct behavior no matter which order events occur in:

  - We update builds after targets complete and after we receive messages, so we're guaranteed to update once both these conditions are true. This means messages can't be lost (even if they arrive before a build completes).
  - The minor changes to the build engine logic mean that firing additional build updates is always safe, no matter what the current state of the build is.
  - The build itself is protected by a lock in the build engine.
  - The target is not covered by an explicit lock, but for all states only the engine (waiting) //or// the worker (all other states) can interact with it. All of the interactions also move the target state forward to the same destination and have no other side effects.
  - Messages are only consumed inside the engine lock, so they don't need an explicit lock.

Test Plan:
  - Made an HTTP request wait after completion, then ran a pile of builds through it using `bin/harbormaster build` and the web UI.
  - Passed and failed message-awaiting builds with `harbormaster.sendmessage`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, zeeg

Differential Revision: https://secure.phabricator.com/D8788
2014-04-16 13:01:46 -07:00
epriestley
803c50c1e7 Allow Harbormaster HTTP steps to pass credentials
Summary: Fixes T4590. Use the credentials custom field to allow Harbormaster HTTP requests to include usernames/passwords.

Test Plan: Ran a build plan with credentials, verified they were sent to the remote server.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4590

Differential Revision: https://secure.phabricator.com/D8786
2014-04-16 13:01:38 -07:00
epriestley
c402d7d307 Implement smart waits for rarely updated repositories
Summary:
Ref T4605. When figuring out how long to wait to update a repository, factor in when it was last pushed. For rarely updated repositories, wait longer between updates.

(A slightly funky thing about this is that empty repos update every 15 seconds, but that seems OK for the moment.)

Test Plan:
Ran `bin/phd debug pulllocal` and saw sensible calculations and output:

```
...
<VERB> PhabricatorRepositoryPullLocalDaemon Last commit to repository "rPOEMS" was 1,239,608 seconds ago; considering a wait of 6,198 seconds before update.
>>> [79] <query> SELECT * FROM `repository` r   ORDER BY r.id DESC
<<< [79] <query> 514 us
>>> [80] <query> SELECT * FROM `repository_statusmessage` WHERE statusType = 'needs-update'
<<< [80] <query> 406 us
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIH" is not due for an update for 8,754 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rDUCK" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rMTESTX" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rQWER" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rBT" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rSVNX" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIG" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rHGTEST" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rBTX" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rGX" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rMTX" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPOEMS" is not due for an update for 6,198 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPHU" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rSVN" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPHY" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rGTEST" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIS" is not due for an update for 6,894 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rARCLINT" is not due for an update for 21,599 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rLPHX" is not due for an update for 1,979 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rARC" is not due for an update for 1,824 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIHG" is not due for an update for 21,599 second(s).
...
```

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4605

Differential Revision: https://secure.phabricator.com/D8782
2014-04-16 13:01:04 -07:00
epriestley
5671c4b276 Make PullLocal daemon more flexible and transparent about scheduling
Summary:
Ref T4605. Fixes T3466. The major change here is that we now run up to four simultaneous updates. This should ease cases where, e.g., one very slow repository was blocking other repositories. It also tends to increase load; the next diff will introduce smart backoff for cold repositories to ease this.

The rest of this is just a ton of logging so I can IRC debug these things by having users run them in `phd debug pulllocal` mode.

For T3466:

  - You now have to hit four simultaneous hangs to completely block the update process.
  - Importing repository updates are killed after 4 hours.
  - Imported repository updates are killed after 15 minutes.

Test Plan:
  - Ran `phd debug pulllocal` and observed sensible logs and behavior.
  - Interrupted daemon from sleeps and processing with `diffusion.looksoon`.
  - Ran with various `--not`, `--no-discovery` flags.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3466, T4605

Differential Revision: https://secure.phabricator.com/D8785
2014-04-16 13:00:49 -07:00
epriestley
417056932e Make discovery slightly cheaper in the common case
Summary:
Ref T4605. Before discovering branches, try to prefill the cache in bulk. For repositories with large numbers of branches, this allows us to issue dramatically fewer queries.

(Before D8780, this cache was usually held across discovery events, so being able to fill it cheaply was not as relevant.)

Test Plan: Ran discovery on Git, Mercurial and SVN repositories. Observed fewer queries for Git/Mercurial.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4605

Differential Revision: https://secure.phabricator.com/D8781
2014-04-16 13:00:38 -07:00
epriestley
118c696f72 Separate repository updates from the pull daemon
Summary:
Ref T4605. Currently, the PullLocal daemon is responsible for two relatively distinct things:

  - scheduling repository updates; and
  - actually updating repositories.

Move the "actually updating" part into a new `bin/repository update` command, which basically runs the pull, discover, refs and mirror commands. This will let the parent process focus on scheduling in a more understandable way and update multiple repositories at once. It also makes it easier to debug and understand update behavior since the non-scheduling pipeline can be run separately.

Test Plan:
  - Ran `update --trace` on SVN, Mercurial and Git repos.
  - Ran PullLocal daemon for a while without issues.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4605

Differential Revision: https://secure.phabricator.com/D8780
2014-04-16 13:00:29 -07:00
Chad Little
2cf2117eee Remove extra workboard margin on mobile
Summary: We have too much space on workboards when displayed on mobile devices.

Test Plan: Shrink browser display, note that all workboards align to common gutters.

Reviewers: epriestley, btrahan

Reviewed By: btrahan

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8790
2014-04-16 11:09:53 -07:00
Chad Little
f5eb7b6404 Add ability to edit Projects on mobile
Summary: sets action list to crumbs

Test Plan: shrink browser, see mobile action list, click on it, edit

Reviewers: epriestley, btrahan

Reviewed By: btrahan

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8789
2014-04-16 11:06:04 -07:00
epriestley
041c3fd19d Let project prefilling accept PHIDs
Summary: I recently made this better about accepting project names, but we use it in some cases with PHIDs. Make that work properly again.

Test Plan: Clicked "New Task" from a project page.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8778
2014-04-15 11:17:37 -07:00
epriestley
20abf37b88 Fix a lookup issue in Owners
Summary:
Fixes T4477. Sort of winging this but it's probably the right fix?

One error in T4477.

One error via email:

```
[2014-04-15 17:44:34] ERROR 8: Undefined index: /some_index/ at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:213]
  #0 PhabricatorOwnersPackage::findLongestPathsPerPackage(Array of size 3 starting with: { 0 => Array of size 3 starting with: { id => 5 } }, Array of size 8 starting with: { / => Array of size 2 starting with: { /some_index/some_file.py => true } }) called at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:170]
  #1 PhabricatorOwnersPackage::loadPackagesForPaths(Object PhabricatorRepository, Array of size 2 starting with: { 0 => /some_index/some_file.py }) called at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:119]
...
```

Test Plan: Will make @zeeg do it.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, zeeg

Maniphest Tasks: T4477

Differential Revision: https://secure.phabricator.com/D8779
2014-04-15 11:12:42 -07:00
Bob Trahan
c845b757ee Maniphest - remove "attach file" action
Summary: Fixes T4655. Basically leaves the display code intact for legacy installs but removes the option from the UI and removes "create" code.

Test Plan:
tried to attach file and the action was not in the dropdown!
made a new task and it worked!
commented on an old task and it worked!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4655

Differential Revision: https://secure.phabricator.com/D8777
2014-04-15 10:49:05 -07:00
epriestley
cb545856a9 Make task queue more robust against long-running tasks
Summary:
See discussion in D8773. Three small adjustments which should help prevent this kind of issue:

  - When queueing followup tasks, hold them on the worker until we finish the task, then queue them only if the work was successful.
  - Increase the default lease time from 60 seconds to 2 hours. Although most tasks finish in far fewer than 60 seconds, the daemons are generally stable nowadays and these short leases don't serve much of a purpose. I think they also date from an era where lease expiry and failure were less clearly distinguished.
  - Increase the default wait-after-failure from 60 seconds to 5 minutes. This largely dates from the MetaMTA era, where Facebook ran services with high failure rates and it was appropriate to repeatedly hammer them until things went through. In modern infrastructure, such failures are rare.

Test Plan:
  - Verified that tasks queued properly after the main task was updated.
  - Verified that leases default to 7200 seconds.
  - Intentionally failed a task and verified default 300 second wait before retry.
  - Removed all default leases shorter than 7200 seconds (there was only one).
  - Checked all the wait before retry implementations for anything much shorter than 5 minutes (they all seem reasonable).

Reviewers: btrahan, sowedance

Reviewed By: sowedance

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8774
2014-04-15 08:42:02 -07:00
Peng Li
6a4f126000 Give the commitownersparser a little more time
Summary:
Recently we see issues with huge commits (branch cuts for www) where people received hundreds of emails for the same commit. By checking all the active and archived tasks related to such commits, I saw the following pattern:
 - The commit itself is marked as importStatus = 15 which means all the processing was actually done;
 - In archived tasks, I see one PhabricatorRepositorySvnCommitMessageParserWorker, one PhabricatorRepositorySvnCommitChangeParserWorker, followed by many PhabricatorRepositoryCommitHeraldWorker, which means that the PhabricatorRepositoryCommitOwnersWorker (who schedule those herald tasks) was never done;
 - PhabricatorRepositoryCommitOwnersWorker is always active (for days) with failureCount = 0;
 - In daemon log I see a lot of lease expire exception for PhabricatorRepositoryCommitOwnersWorker.
So to me it looks like the following happened:
 - Everything is fine until we schedule the PhabricatorRepositoryCommitOwnersWorker
 - PhabricatorRepositoryCommitOwnersWorker actually successfully finished but its running time exceed 60s. Before it finishes, it scheduled the PhabricatorRepositoryCommitHeraldWorker task
 - When we try to archive it, the lease expiration exception happened. As a result, it stayed active and will be picked up immediately since it is in the head of the queue
 - The two steps above repeat forever until we kill it
I am not sure why we want to check lease expiration when we are archiving the task. For now I am giving the worker a little more time since parsing half million affected path needs some time..

Test Plan: Patched in our production and it worked.

Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8773
2014-04-14 15:52:02 -07:00
epriestley
47245b1b59 Remove some ad-hoc loading of repositories from Releeph
Summary: Ref T3551. Since we now require repositories in order to perform policy checks, things that did loads properly don't need to load this data explicitly.

Test Plan: Edited a product, cut a new branch.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

Differential Revision: https://secure.phabricator.com/D8769
2014-04-14 12:08:04 -07:00
epriestley
f4c8a34abe Remove several "loadArcanistProject()" methods
Summary:
Ref T3551. Releeph has old-style `loadX()` methods; get rid of one of them.

Differential has a couple of copies of this too, clean them up.

Test Plan:
  - Viewed various differential revisions (with and without projects).
  - Viewed and edited Releeph products.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

Differential Revision: https://secure.phabricator.com/D8768
2014-04-14 12:07:32 -07:00
epriestley
05e30a024d Unban releeph product name "branch"
Summary:
Fixes T3657. We no longer construct ambiguous URIs, so product names are no longer restricted.

Also fix some minor URI construction stuff.

Test Plan: Created a product called "branch".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3657

Differential Revision: https://secure.phabricator.com/D8767
2014-04-14 12:07:14 -07:00
epriestley
6deab24475 Remove ReleephProjectController
Summary:
Ref T3657. General changes here:

  - Removes `ReleephProjectController`, which is the source of T3657.
  - Mostly moves requests from "RQ" as a monogram to "Y" (looks like a merge, mnemonic for "yank"?, we don't have too many characters left). This should be essentially only cosmetic. This reduces ambiguity with "rQ" and "R123", which are current and future repository monograms. This will continue in the next few diffs.
  - Makes requests implement policies correctly.

Test Plan: Created, edited, browsed requests.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3657

Differential Revision: https://secure.phabricator.com/D8766
2014-04-14 12:06:56 -07:00
epriestley
6899fbcf29 Add DifferentialHunkQuery to start hiding hunk storage details
Summary:
Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.

This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.

Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4045

Differential Revision: https://secure.phabricator.com/D8765
2014-04-14 12:06:26 -07:00
epriestley
aaf1320b02 Simplify Herald logic for loading Differential changes
Summary: Ref T4045. These three methods are fairly copy-pastey. Provide a more formal DifferentialHunk API for querying various types of line ranges.

Test Plan: Used test console to verify that "added content", "removed content", and "changed content" rules still produce the same data.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4045

Differential Revision: https://secure.phabricator.com/D8764
2014-04-14 12:06:20 -07:00
epriestley
2712091a71 Move Releeph branch controllers toward a modern/stable state
Summary:
Ref T3644. Ref T3657. Ref T3549. Basically:

  - Move these controllers to modern query/policy infrastructure.
  - Move them to consistent, ID-based URIs.
  - Rename "Project" to "Product"; "Pick Request" to "Pull Request".
  - Clean up a few UI things here and there.

Test Plan:
  - Created and edited branches.
  - Opened and closed branches.
  - Viewed branch history.
  - Searched within a branch.
  - Browsed to branches from products.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3644, T3549, T3657

Differential Revision: https://secure.phabricator.com/D8646
2014-04-12 16:53:51 -07:00
Bob Trahan
cb675a46a2 Differential - fix bug writing affected paths
Summary: Fixes T4774. With the new code and configuration instructions downplaying the role of arcanist project we weren't writing affected paths at all! I had this issue on my installation - no affected paths were written. We seem to always have the repository now though if we can see it, so not too bad of a fix.

Test Plan: updated a diff and was able to browse in diffusion.

Reviewers: epriestley, bitglue

Reviewed By: epriestley, bitglue

Subscribers: bitglue, epriestley, Korvin

Maniphest Tasks: T4774

Differential Revision: https://secure.phabricator.com/D8757
2014-04-11 12:54:21 -07:00
Bob Trahan
f86ab666f6 Differential - make diffs you authored + are reviewer for show up in appropos boxes
Summary: Fixes T2328. Note the audit part is fixed now.

Test Plan: Tried to reproduce the audit issue by raising my own commit as a problem; it showed up before code changes! Made a diff with my self as author and reviewer; it showed up as expected

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T2328

Differential Revision: https://secure.phabricator.com/D8755
2014-04-11 10:31:07 -07:00
epriestley
499f66963d Minor, fix the scoping of a static variable
Auditors: chad, btrahan
2014-04-11 04:24:58 -07:00
Ben Alpert
ac0ad2e016 Fix typo in variable name
Summary: Follow-up to D8758.

Test Plan: Crossed fingers.

Reviewers: #blessed_reviewers, chad, epriestley

Reviewed By: #blessed_reviewers, chad, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8759
2014-04-11 04:18:24 -07:00
Bob Trahan
524e8a64f4 Countdown - use better date control
Summary: Fixes T3576

Test Plan: made a countdown and it looked right on view. edited it and it had the right values pre and post edit.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T3576

Differential Revision: https://secure.phabricator.com/D8754
2014-04-10 18:14:47 -07:00
epriestley
650bc97ed7 Don't load every commit if there are no local hashes
Summary: We make a silly query for every commit if you copy/paste a diff.

Test Plan: Copy/pasted diffs now render in fewer than 30 seconds.

Reviewers: btrahan, spicyj

Reviewed By: btrahan, spicyj

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8758
2014-04-10 18:09:54 -07:00
Joshua Spence
e7419d85f7 Set celerity.minify true in production environments.
Summary: I haven't been able to understand why this isn't set by default in production environments (since it is recommended to do so anyway).

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8743
2014-04-10 14:39:05 -07:00
Bob Trahan
3e0b3a1db5 Feed - fix some whacky "text mode" rendering code
Summary: ...add a "renderingTarget" to FeedStory and use it as appropos. Overall, not a ton of changes was necessary to make this work. I think this could be made to be even cleaner by going through each and every feed story and re-implementing as necessary with the full toolset available. But this is good enough for now I think, and just something to keep cleaning up when we're in here. Fixes T4630.

Test Plan: made a task. gave it a token. viewed my feed - saw stories. used conduit.feed.query with mode == 'text' and got good readable results.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: spicyj, epriestley, Korvin

Maniphest Tasks: T4630

Differential Revision: https://secure.phabricator.com/D8750
2014-04-10 13:46:02 -07:00
Bob Trahan
4b56dbed3a Herald - make tokenizers have the purdy icons
Summary: ...use the prefab stuff as it does fancier things than we were doing. Only trick then really is to pass username and the map of handle phids => icons to the client so prefab can work nicely. Fixes T4775.

Test Plan: made a herald rule with projects and users. Saw nice icons. Reloaded page and still saw nice icons.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4775

Differential Revision: https://secure.phabricator.com/D8749
2014-04-10 12:38:15 -07:00
epriestley
facd3ea8ef Modernize chatlog a bit
Summary:
Ref T4786. This doesn't fully fix the issue since there's no way to make channels public yet, but gets some of the infrastructure more up to date.

  - Allow public access to the list and log controllers.
  - Implement proper policy checks in the Events (this has no practical impact on the only controller that loads this stuff, it's just for general/future purposes).
  - Remove a old-style unused method for building page frames.

Test Plan: Viewed log list and log details as logged-in and logged out users.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4786

Differential Revision: https://secure.phabricator.com/D8746
2014-04-10 11:45:21 -07:00
epriestley
ab7d89edc8 Use better secrets in generating account tokens
Summary:
When we generate account tokens for CSRF keys and email verification, one of the inputs we use is the user's password hash. Users won't always have a password hash, so this is a weak input to key generation. This also couples CSRF weirdly with auth concerns.

Instead, give users a dedicated secret for use in token generation which is used only for this purpose.

Test Plan:
  - Ran upgrade scripts.
  - Verified all users got new secrets.
  - Created a new user.
  - Verified they got a secret.
  - Submitted CSRF'd forms, they worked.
  - Adjusted the CSRF token and submitted CSRF'd forms, verified they don't work.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8748
2014-04-10 11:45:10 -07:00
Brayden Winterton
b660960771 Add a priorityColor property to the maniphest conduit endpoint
Summary:
I added a getTaskPriorityColor function to the ManiphestTaskPriority class which returns the color set in the maniphest config for the given priority.

This is in preparation for a change to arcanist which will allow it to display the priority color (if it is a supported color) upon running `arc tasks`.

Fixed some linting issues

Test Plan:
Invoke the maniphest.info method from conduit and ensure that:
 * The priorityColor property is given in the json
 * the priorityColor property is set correctly

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8734
2014-04-09 11:32:08 -07:00
Bob Trahan
2d43cf1296 OAuth - add a little notes section for admins to remember details about external accounts
Summary: Fixes T4755. This also includes putting in a note that Google might ToS you to use the Google+ API. Lots of code here as there was some repeated stuff between OAuth1 and OAuth2 so I made a base OAuth with less-base OAuth1 and OAuth2 inheriting from it. The JIRA provider remains an independent mess and didn't get the notes field thing.

Test Plan: looked at providers and read pretty instructions.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4755

Differential Revision: https://secure.phabricator.com/D8726
2014-04-09 11:09:50 -07:00
epriestley
8b3eced0c7 Make Maniphest project prefill more modern and standard
Summary: Fixes T4777. We technically support `?projects=...` already, but parse it in an unusual way and apply old, awkward, excessively strict lookups to it.

Test Plan: Used reasonable, standard, human-readable strings to prefill `?projects=` and got the results I expected.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4777

Differential Revision: https://secure.phabricator.com/D8733
2014-04-09 08:37:23 -07:00
Brayden Winterton
dffbbaf0a6 Added isClosed property to maniphest conduit endpoint in order to fix an issue with arcanist when displaying tasks
Summary:
Arcanist is currently displaying all tasks as closed when invoking `arc tasks`.
This is because arcanist is setting the display to closed if there is anything in the `status` property. Adding an isClosed property will allow arcanist to properly display open/closed status on tasks by checking against the isClosed property. The isClosed property will be set according to the closed property that is set on each status in maniphest.

Test Plan:
Invoke the conduit maniphest.info method on any task and insure that:
 # The isClosed property is included in the properties
 # that it is set properly according to the statuses set for maniphest.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4744

Differential Revision: https://secure.phabricator.com/D8731
2014-04-09 07:47:31 -07:00
lkassianik
01552d85de Show Projects bucket unconditionally in repository summary screen
Summary: fixes T4753

Test Plan: looked at repository with projects, looked at repository with no projects

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4753

Differential Revision: https://secure.phabricator.com/D8730
2014-04-08 19:48:31 -07:00
Ben Alpert
a7272dfb03 Switch back to zwsp for oncopy line marker
Summary:
Fixes T4759.

Turns out Chrome on windows doesn't really like the word joiner character. We'll switch back to zwsp but make it `position: absolute;` so it doesn't turn into a line break.

Test Plan: Looked at diffs in IE9 and Chrome Windows. Made sure copying still works as expected.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4759

Differential Revision: https://secure.phabricator.com/D8727
2014-04-08 17:55:48 -07:00
epriestley
563a1ac5bf Fix parsing and storage of generated SSH keys
Summary: Fixes T4772. We weren't parsing generated public keys properly, and were storing them in the wrong format.

Test Plan:
  - Updated a private key.
  - Generated a public key.
  - Saved the public key.
  - Used a generated private key to authenticate.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4772

Differential Revision: https://secure.phabricator.com/D8721
2014-04-08 14:52:37 -07:00
epriestley
f79320e64e Render default/current settings in the same format they'll be edited
Summary:
Fixes T4773. For config settings of type `list<string>`, `set`, or `list<regex>`, the "defaults" table and "examples" aren't always in the same format you should actually use when changing the setting.

This is pretty confusing. Instead, always show the settings in the desired format. For example, if the user should enter a newline-separated list, show them a newline separated list.

Test Plan:
  - Grepped for `list<string>`, `list<regex>`, and `'set'`; verified all the config had the right example format (most already did).
  - Viewed config settings of various kinds, including custom settings.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4773

Differential Revision: https://secure.phabricator.com/D8725
2014-04-08 12:09:43 -07:00
Bob Trahan
d5ded805b2 Herald - fix change type bug
Summary: wasn't working due to some type issues. Fixes T4756. I also made it display nicer while I was debugging this.

Test Plan: created a herald rule to block changes that added refs. git tag -a "test" -m "test test"; git push origin test got me blocked!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4756

Differential Revision: https://secure.phabricator.com/D8724
2014-04-08 11:58:28 -07:00
epriestley
582ec54465 Add a checkbox to the LDAP auth configuration UI to "Always Search"
Summary: Fixes T3208. This forces us to bind+search even if there are no anonymous credentials.

Test Plan: Checked the box, saved the form. Unchecked the box, saved the form. LDAP??

Reviewers: Firehed

Reviewed By: Firehed

Subscribers: epriestley

Maniphest Tasks: T3208

Differential Revision: https://secure.phabricator.com/D8723
2014-04-08 11:36:23 -07:00
epriestley
b937bf5946 Spell "eventually" correctly, eventually.
Auditors: btrahan
2014-04-08 10:26:33 -07:00
epriestley
e8e62f82ce Hide "Land to hosted git" button for now
Summary: Ref T182. This feature rarely/never works and is on the balance enormously confusing to users (see <https://github.com/facebook/phabricator/issues/566>). If installs have somehow made it sort of work, they can comment this line out for now until we have time to make this work more reasonably.

Test Plan: Looked at a revision in Differential.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D8719
2014-04-08 08:05:28 -07:00
epriestley
bd0011076b Don't disrupt repository import chain when publishing is disabled
Summary:
Fixes T4736. Currently, we incorrectly skip the `writeImportStatusFlag()` call if publishing is disabled (the `herald-disabled`) check. This means we don't flag the commit as imported, and don't move the pipeline forward correctly.

Instead, we only want to skip the owners stuff, not the pipeline stuff. Move that to a method.

(Also fix a nearby TODO now that we have a permanent failure exception.)

Test Plan:
  - Used `scripts/repository/reparse.php --owners ...` to execute this code, fiddled with things to hit both the disabled and enabled branches and verified the flag stuff is still reached.
  - Faked the exceptions and made sure they raise correctly.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4736

Differential Revision: https://secure.phabricator.com/D8715
2014-04-08 05:13:28 -07:00
Ben Alpert
cd829434d4 Include storyText in feed POST data
Summary: I also changed PhabricatorApplicationTransactionFeedStory and the TokenGivenFeedStory to include only the title/first line of the feed story, which is more convenient (previously, strip_tags gave a multi-line story without even any linebreaks) and more consistent with the other story types.

Test Plan: Added a requestbin URL to feed.http-hooks, commented on a Differential, and saw storyText equal to "alpert added a comment to D2: c." in the POST data it received.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Maniphest Tasks: T4630

Differential Revision: https://secure.phabricator.com/D8710
2014-04-06 11:48:39 -07:00
epriestley
b03912213f Add original mail headers to the bottom of error mail
Summary: From IRC, this is sometimes helpful for debugging if there's a mailing list issue or something like that. For example, it can show "To" and "Cc".

Test Plan: Got some email, saw headers in it.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8708
2014-04-06 07:57:03 -07:00
Chad Little
3b33089de7 Don't show 'needs review' on DifferentialObjects
Summary:
This text is overly repetitive and is not super important. Keeps the other states. Also

- Easier to parse reviewers now
- Mobile is less janky

Test Plan:
reload my list of diffs

{F138756}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8707
2014-04-04 14:00:28 -07:00
Bob Trahan
c408168c25 Diffusion - Warn users to explicitly provide PATH for SVN hosted repositories
Summary: Fixes T4547.

Test Plan: saw the warning, looked good

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4547

Differential Revision: https://secure.phabricator.com/D8706
2014-04-04 12:47:10 -07:00
Bob Trahan
f67a853fe7 Audit - add ability to add a package as an auditor
Summary: Fixes T4687. This was also pretty easy...!

Test Plan: made a package with a test user as owner. added package as owner. looked right on commit page. logged in as test user and verified audit showed up on home page.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4687

Differential Revision: https://secure.phabricator.com/D8705
2014-04-04 12:25:03 -07:00
Bob Trahan
496a7d8967 Transactions - make edit transactions that are grouped work nicely
Summary: ...the key is to move a layer lower and beam down the updated comment. There is a wee bit of Javascript gymnastics going on here. Fixes T4608.

Test Plan: made a comment + resolve. clicked edit and made changes. noted transaction updated correctly and "history" link worked. edited again to a deletion and noted the "this is deleted" looked right and history link still worked

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Maniphest Tasks: T4608

Differential Revision: https://secure.phabricator.com/D8702
2014-04-04 12:23:22 -07:00
Bob Trahan
6b5308c981 Audit - add ability to add user or projects as auditors
Summary: Ref T4687. Trickier part is adding packages; will require some typeahead core changes

Test Plan: add a project as an auditor succuessfully!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4687

Differential Revision: https://secure.phabricator.com/D8704
2014-04-04 11:29:10 -07:00
epriestley
c8cf7bb506 Simplify some more older mail error handling code
Summary: Ref T4371. We can reuse more code for this "your stuff is empty" error, now, and benefit from global rate limiting and being able to reply to arbitrary addresses.

Test Plan: Sent valid, empty, and empty-ignored email via `mail_handler.php`, got appropriate actions/errors/states.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4371

Differential Revision: https://secure.phabricator.com/D8701
2014-04-04 11:14:33 -07:00
epriestley
2f01214322 Restore reply instructions to ApplicationTransaction email
Summary: This "Reply to comment, etc., etc." section got lost along the way at some point. Restore it for transaction mail.

Test Plan: Received mail from Maniphest with reply instructions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8700
2014-04-04 11:14:21 -07:00
epriestley
7f81fd4bf4 Make contextual matching of project monograms more liberal
Summary:
We currently share the same regexp between PHID matching (usually unambiguous) and remarkup matching (often ambiguous).

This means that some project monograms which should work fine don't work properly in some contexts. Improve these behaviors.

For example:

  - `#domain.com`
    - Previously did not work at all.
    - Now works in unambiguous cases, and in remarkup.
  - `#1`
    - Previously did not work at all.
    - Now works in unambiguous cases.
  - `#dot.`
    - Previously did not work at all.
    - Now works in unambiguous cases.

Test Plan:
  - Created projects `domain.com`, `1`, etc.
  - Used jump nav to match them unambiguously, everything worked.
  - Used remarkup preview to match them ambiguously, the reasonable ones worked.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8703
2014-04-04 11:13:06 -07:00
epriestley
e3b5737d02 Support CustomField in Herald, mostly
Summary: Ref T655. Ref T418. This mostly supports CustomFields in Herald, for conditions only.

Test Plan: {F137845}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T418, T655

Differential Revision: https://secure.phabricator.com/D8695
2014-04-03 18:43:49 -07:00
epriestley
d9cdbdb9fa When we fail to process mail, tell the user about it
Summary:
Ref T4371. Ref T4699. Fixes T3994.

Currently, we're very conservative about sending errors back to users. A concern I had about this was that mistakes could lead to email loops, massive amounts of email spam, etc. Because of this, I was pretty hesitant about replying to email with more email when I wrote this stuff.

However, this was a long time ago. We now have Message-ID deduplication, "X-Phabricator-Sent-This-Mail", generally better mail infrastructure, and rate limiting. Together, these mechanisms should reasonably prevent anything crazy (primarily, infinite email loops) from happening.

Thus:

  - When we hit any processing error after receiving a mail, try to send the author a reply with details about what went wrong. These are limited to 6 per hour per address.
  - Rewrite most of the errors to be more detailed and informative.
  - Rewrite most of the errors in a user-facing voice ("You sent this mail..." instead of "This mail was sent..").
  - Remove the redundant, less sophisticated code which does something similar in Differential.

Test Plan:
  - Using `scripts/mail/mail_receiver.php`, artificially received a pile of mail.
  - Hit a bunch of different errors.
  - Saw reasonable error mail get sent to me.
  - Saw other reasonable error mail get rate limited.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3994, T4371, T4699

Differential Revision: https://secure.phabricator.com/D8692
2014-04-03 18:43:18 -07:00
Ben Alpert
f9a92c7631 Sort inline comments by id in case of ties
Summary: This ensures that two comments by the same author on the same line are sorted properly.

Test Plan: Before this patch, made two comments that appeared in the wrong order. With this patch, they sort correctly.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8697
2014-04-03 18:41:58 -07:00
Ben Alpert
133772d7eb Set from name when $can_send_as_user is true
Test Plan: Crossed fingers.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8694
2014-04-03 17:51:23 -07:00
Ben Alpert
a0ac3d6716 Add colgroup in differential "shield" tables
Summary: Fixes minor brokenness from D8686.

Test Plan: Expanded a generated file, saw a normal-looking diff.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8693
2014-04-03 15:50:02 -07:00
Chad Little
7446402f92 Differential Action List on Mobile
Summary: Make the actions appear in crumbs on mobile

Test Plan: Test action list on a mobile diff layout

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4730

Differential Revision: https://secure.phabricator.com/D8691
2014-04-03 12:01:04 -07:00
epriestley
c9311a9eae Make errors in dialogs look reasonable instead of hideous
Summary: I accidentally made these exceptionally ugly recently.

Test Plan: {F137411}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley, chad

Differential Revision: https://secure.phabricator.com/D8684
2014-04-03 11:23:03 -07:00
epriestley
957b9c1729 Restore Maniphest status transactions for burnup chart purposes
Summary: The "burnup chart" relies on these to determine when tasks opened and we recently stopped writing them. Keep writing them for now. They're fluff and don't show up in the UI, but draw the right chart.

Test Plan: Saw chart go up when I made tasks.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8682
2014-04-03 11:22:47 -07:00
epriestley
847b7977c1 Add semi-generic rate limiting infrastructure
Summary:
This adds a system which basically keeps a record of recent actions, who took them, and how many "points" they were worth, like:

  epriestley email.add 1 1233989813
  epriestley email.add 1 1234298239
  epriestley email.add 1 1238293981

We can use this to rate-limit actions by examining how many actions the user has taken in the past hour (i.e., their total score) and comparing that to an allowed limit.

One major thing I want to use this for is to limit the amount of error email we'll send to an email address. A big concern I have with sending more error email is that we'll end up in loops. We have some protections against this in headers already, but hard-limiting the system so it won't send more than a few errors to a particular address per hour should provide a reasonable secondary layer of protection.

This use case (where the "actor" needs to be an email address) is why the table uses strings + hashes instead of PHIDs. For external users, it might be appropriate to rate limit by cookies or IPs, too.

To prove it works, I rate limited adding email addresses. This is a very, very low-risk security thing where a user with an account can enumerate addresses (by checking if they get an error) and sort of spam/annoy people (by adding their address over and over again). Limiting them to 6 actions / hour should satisfy all real users while preventing these behaviors.

Test Plan:
This dialog is uggos but I'll fix that in a sec:

{F137406}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8683
2014-04-03 11:22:38 -07:00
Ben Alpert
9fedd343eb Break long words in differential two-up view
Summary: This should prevent long lines from making the code width different between files, which can be annoying. (And of course, it stops long lines from making a giant scrollbar too.)

Test Plan:
Loaded this diff in Chrome, Firefox, IE9, and IE8:

{F137505}

(That's a screenshot from Chrome, but it looks about the same in the other browsers.)

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin, chad

Maniphest Tasks: T2004

Differential Revision: https://secure.phabricator.com/D8686
2014-04-03 09:40:00 -07:00
James Rhodes
85394a9e9e Set name parameter when saving file via Drydock
Summary: This sets the name parameter when Drydock uploads a file so that the storage engine picks it up correctly.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8673
2014-04-03 09:21:36 -07:00
Chad Little
3005b7a7b1 Mobile Differential Diff Review (2-up)
Summary:
This does two things

 - Modernizes Table of Contents
 - Makes Differential reasonable on mobile

I say resonable, as you still have to scroll horizontal to see the entire diff. This is minor as the rest of the page is 100x more useful. A 1-up view would be preferred, but this is still an improvement.

Test Plan: Used iOS simulator for browsing diffs.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Differential Revision: https://secure.phabricator.com/D8681
2014-04-02 21:49:28 -07:00
Bob Trahan
c6cbff1997 Differential - modernize "Local Commits" table
Summary: ...also link to commits we know about in "Local Commits" and "Revision Update History" tables. Fixes T4585.

Test Plan: made a repo. made a diff (foo) and committed it (bar). made a new diff that was comprised of two local commits. noted links to (bar) in various commit hashes as expected

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Maniphest Tasks: T4585

Differential Revision: https://secure.phabricator.com/D8679
2014-04-02 13:18:11 -07:00
epriestley
af0b749369 Fix many lies in the "User Roles" document
Summary: Fixes T3047. Update this document and remove some lies ("menu bar is read in admin interfaces"!!!!).

Test Plan:
  - Read text.
  - Searched for "System Agent" in the UI and replaced it with "bot" or "bot/script" or similar.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3047

Differential Revision: https://secure.phabricator.com/D8675
2014-04-02 12:06:56 -07:00
epriestley
3294649a02 Make user creation process simpler
Summary:
Fixes T4065. This divides user creation into separate "Standard User" and "Script/Bot" workflows which show only relevant fields and provide guidance.

This fixes the verification mess associated with script/bot users by verifying their email addresses automatically.

Test Plan:
  - Created a standard user.
  - Created a script/bot.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8674
2014-04-02 12:06:27 -07:00
epriestley
1df9a6e6b0 Move "Send Welcome Email" to profiles and nuke old weird edit UI
Summary: Ref T4065. Moves the last of the weird alternate edit UI to profiles. The old "Edit" controller is now for creation only, and the funky pencil icon is gone.

Test Plan: Created accounts; sent welcome email.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8670
2014-04-02 12:06:17 -07:00
epriestley
04b9f94602 Give administrators selective access to System Agent settings panels
Summary: Ref T4065. Give administrators an "Edit Settings" link from profiles, which allows selective edit of settings panels. Enable Conduit, SSH Keys, and VCS Password.

Test Plan:
  - Used these panels for a bot.
  - Used these panels on my own account.
  - Tried to use these panels for a non-bot account, was denied.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8668
2014-04-02 12:06:05 -07:00
epriestley
c9268c4858 Put Disable/Admin flags on profiles
Summary: Ref T4065. Moves the "disable / enable" and "make / unmake administrator" actions to profiles.

Test Plan: Disabled and enabled users, and made and unmade administrators.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8666
2014-04-02 12:05:49 -07:00
epriestley
b6b2e65511 Allow administrators to edit System Agent information from the agent's profile
Summary:
Ref T4065. Currently, we have this super copy/pasted "edit profile picture" UI for system agents.

Instead, give administrators direct access from profiles, so they can use the same code pages do.

Test Plan: Edited my profile picture and profile details. Edited an agent's. Was unable to edit a non-agent user.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8664
2014-04-02 12:05:34 -07:00
epriestley
b53134bf32 Move "Change Username" from weird edit panel to standard object action
Summary: Ref T4065. Make this work in a more standard way which administrators have a reasonable shot at finding and using. See D8662 for discussion.

Test Plan: Changed a user's username.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8663
2014-04-02 12:05:19 -07:00
epriestley
a6a19ac721 Move "Delete User" action to user profiles
Summary:
Ref T4065. The existence of two separate edit workflows for users is broadly confusing to administrators.

I want to unify user administration and improve administration of system agent accounts. Particularly, I plan to:

  - Give administrators limited access to profile editing of system agents (e.g., change profile picture).
  - Give administrators limited access to Settings for system agents.
  - Broadly, move all the weird old special editing into standard editing.

Test Plan:
  - Hit all the errors (delete self, no username, wrong username).
  - Deleted a user.
  - Visited page as a non-admin, got 403'd.
  - Viewed old edit UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8662
2014-04-02 12:05:07 -07:00
epriestley
81fa847bc5 Make "JIRA Issues" field work better with noncredentialed accounts
Summary:
Currently, users get an error when making any changes to this field if they don't have a linked JIRA account.

Instead:

  - We should only raise an error if they're trying to //add// issues, and only on the new issues. It's always fine to remove issues, and existing issues the author can't see are also fine.
  - When we can't add things because there's no account (vs because there's a permissions error or they don't exist), raise a more tailored exception.

Test Plan:
  - As JIRA and non-JIRA users, made various edits to this field.
  - Got appropriate exceptions, including better tailoring.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: mbishopim3, epriestley

Differential Revision: https://secure.phabricator.com/D8676
2014-04-02 12:03:59 -07:00
Bob Trahan
b50426a98f Herald - print out rule monogram rather than rule phid on transcript controller
Summary: Fixes T4632.

Test Plan: viewed a transcript for rule x which depends on rule y and noted "rule y" printed out rather than "PHID-BLAH-BLAH"

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4632

Differential Revision: https://secure.phabricator.com/D8678
2014-04-02 11:59:50 -07:00
Bob Trahan
69b8f56cda Fix maniphest "create" transactions
Summary: the quotes are 'cuz "create" is inferred. Previously, we inferred on "status", but since we set that on "initializeNewTask" instead infer off "title" (aka "name") like most other apps do. Only hairy tweak was to elevate TYPE_TITLE to the most important of all maniphest transactions, which doesn't actually seem too unreasonable if not correct even? Fixes T4686.

Test Plan: made a new task, used bin/mail, got the right headers (mail vary prefix == created)

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4686

Differential Revision: https://secure.phabricator.com/D8639
2014-04-01 14:26:03 -07:00
epriestley
ff98565db6 Fix an issue where email for "accepted" revisions would fatal
Summary: When trying to render "BRANCH", we need the active diff. Load it
in general since it seems reasonable for custom fields to expect it to
exist during mail rendering.
2014-04-01 13:35:01 -07:00
Bob Trahan
750a30d441 Hide uninstalled applications on the "Home Application Tiles" preferences page
Summary: Fixes T4685.

Test Plan: loaded home page settings and saw phrequent, uninstalled phrequent, refreshed, and no longer saw phrequent!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4685

Differential Revision: https://secure.phabricator.com/D8640
2014-04-01 12:27:52 -07:00
epriestley
a6b1fac117 Fix SVN translation of "add-file" protocol frames over SSH
Summary: Fixes T4697. When pushing moved/copied files, SVN sends an "add-file" protocol frame which has a URI in it that needs translation from external format ("/diffusion/X/") to internal format ("/path/to/svn").

Test Plan:
  - Copied/moved files and committed them in SVN.
  - Added files (no copy/move) and committed them in SVN.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4697

Differential Revision: https://secure.phabricator.com/D8654
2014-04-01 08:23:48 -07:00
epriestley
cf6f7446ce Restore "Branch" and "changes since last update" fields to Differential mail
Summary: Ref T418. Fixes T4642. The "changes since last update" and "branch" fields got dropped; restore them in a general, field-driven way.

Test Plan:
  - Created a revision, got relevant sections in mail.
  - Commented on a revision, got relevant sections in mail.
  - Updated a revision, got relevant sections in mail.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: spicyj, epriestley

Maniphest Tasks: T418, T4642

Differential Revision: https://secure.phabricator.com/D8657
2014-04-01 08:23:34 -07:00
epriestley
3aabfc7f4d Fix JIRA integration from "arc" commit messages
Summary: Fixes T4683. This was just a missing method implementation. Also provide a couple of translation things.

Test Plan:
  - Created a revision from the command line with a nonempty `JIRA Issues:` line, via `arc diff`.
  - Looked at the translation strings.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4683

Differential Revision: https://secure.phabricator.com/D8656
2014-03-31 09:27:30 -07:00
Ben Alpert
deba531401 Send mail to audit comment author too
Summary: Previously, you would not receive a mail message for the first comment you make on an audit, but you would for subsequent comments because everyone who's made a comment would be CCed on the email. This mirrors DifferentialTransactionEditor's getMailTo which always adds `$object->getAuthorPHID()`.

Test Plan: With self mail turned on, made the first comment on a commit and received an email for it. With self mail turned off, commented on a different commit and saw in `bin/mail list-outbound` that the message was voided.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8650
2014-03-31 07:52:51 -07:00
Chad Little
42da874691 Update Phriction History UI
Summary: Uses cards, fixes bgcolors.

Test Plan: View edit history on a few documents.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Differential Revision: https://secure.phabricator.com/D8648
2014-03-30 11:18:49 -07:00
Carl Jackson
213eea7bdd Fix Mailgun Reply-To handling
Summary:
When sending the "Reply-To" header to Mailgun, Phabricator would
previously send two headers for every "Reply-To": "Reply-To[0][email]" and
"Reply-To[0][name]". Instead, explicitly build the header as specified by RFC
2822 and send it to Mailgun pre-baked.

Pretty sure this bug was a cargo-cult from the Sendgrid code, where (apparently)
this actually works.

Test Plan:
Triggered an email from Phabricator, saw that the header was sent
properly.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8645
2014-03-29 10:53:52 -07:00
Chad Little
cca5078d9f Make attention count yellow
Summary:
It needs attention! Serious color for serious action.

Also, Flags probably need urgent action!1!

Test Plan: Reload Hompage, see new color

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Differential Revision: https://secure.phabricator.com/D8644
2014-03-29 10:26:53 -07:00
epriestley
8c88180ee1 Rename project -> product on edit/create UIs
Summary: Ref T3092. Ref T3549. Modernize the product creation and edit UIs and make them say "product" instead of "project".

Test Plan:
  - Created products.
  - Edited products.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3092, T3549

Differential Revision: https://secure.phabricator.com/D8636
2014-03-29 09:16:40 -07:00
epriestley
98b0b5e62b Rename more project -> product in Releeph
Summary:
Ref T3549.

  - Renames most "project" in view/list controllers.
  - Moves away from "ProjectController".
  - Modernizes some modern UI.

Test Plan:
  - Viewed product lists.
  - Viewed product detail pages.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549

Differential Revision: https://secure.phabricator.com/D8635
2014-03-29 09:16:24 -07:00
epriestley
c7bcecb9b0 Use ApplicationTransactions for Releeph product activity
Summary:
Ref T3549. Ref T3663.

  - Use transactions for activate/deactivate.
  - Rename some "project" -> "product".

Test Plan:
  - Activated products.
  - Deactivated products.

{F135480}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3663, T3549

Differential Revision: https://secure.phabricator.com/D8634
2014-03-29 09:16:02 -07:00
epriestley
d3dbbec88d Rename Releeph "Project" transactions to "Product"
Summary: Ref T3549. This table isn't written to yet; rename it and the DAOs and modernize the history controller.

Test Plan: Viewed history page for a product.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549

Differential Revision: https://secure.phabricator.com/D8633
2014-03-29 09:15:09 -07:00
epriestley
a5ad923573 Modernize Releeph "Product Activate" controller
Summary:
Ref T3549. A few things here:

  - Releeph has an object called a "Project". We'd like to call this a "Product" instead. See T3549. Rename easy instances that don't break URIs.
  - Releeph has a "ProjectController" which tries to be smart about loading objects. However, it's big and messy and doesn't have the finesse to do policies or `needX(...)` correctly. It also generates URIs which collide with one another. Introduce "ProductController" to start to move away from it.
  - Some small modernizations to this controller to take advantage of newer infrastructure (like easier dialog rendering).

Test Plan: Deactivated and reactivated products.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3549

Differential Revision: https://secure.phabricator.com/D8632
2014-03-29 09:14:44 -07:00
epriestley
6c8cef3bee Remove ReleephUserView
Summary:
Fixes T3738. Facebook uses this to provide a couple of integrations (push karma, is user an intern?), but the mechanism is both very complex and not very general.

Instead, these features are better implemented in Hovercards or via CustomField. We'll help Facebook integrate things when the time comes, but per discussion in T3738 none of this is critical or especially complicated.

Test Plan:
  - Grepped for all callsites.
  - Viewed a request and verified that author/requestor populated and rendered correctly.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T3738

Differential Revision: https://secure.phabricator.com/D8631
2014-03-29 09:14:32 -07:00
epriestley
ce5714526a Remove Releeph "Authors" rules
Summary:
Fixes T3659. Releeph has some awkward complexity around who ends up as a commit author. Instead, we should always try to use the original author.

Metadata (like the requestor's identity) should be accessed via Conduit or other channels instead.

Test Plan: Saved some projects, grepped for all related symbols.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T3659

Differential Revision: https://secure.phabricator.com/D8630
2014-03-29 09:14:08 -07:00
epriestley
4f26b8ffcf Remove "Release Counter" from Releeph
Summary: Fixes T3658. This field doesn't make much sense and doesn't appear to ever have actually been implemented. Particularly, the `%N` pattern doesn't actually work and I can't find anything which actually calls this stuff or exposes it externally. Facebook doesn't use it (see T3658) and I don't think it's useful in general.

Test Plan: Used `grep` to look for stuff, edited a project.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T3658

Differential Revision: https://secure.phabricator.com/D8628
2014-03-29 09:13:45 -07:00
Chad Little
cc626b0306 Add pager to all Audit Views
Summary: Fixes T1153, removes the "active" restriction from pagination

Test Plan: Bring up audit, test pager exists, and doesn't exist when needed

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aik099, epriestley, Korvin

Maniphest Tasks: T1153

Differential Revision: https://secure.phabricator.com/D8643
2014-03-29 08:43:17 -07:00
Bob Trahan
44942f3378 Fix help menu links for folks with diviner uninstalled
Summary: move code to uninstallable help app rather than diviner. Fixes T4690.

Test Plan: uninstalled diviner, noted no links, then moved the code and suddenly helpful help links showed up once more.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4690

Differential Revision: https://secure.phabricator.com/D8638
2014-03-28 13:41:19 -07:00
Bob Trahan
8d41d9d8f8 Fix Maniphest links w.r.t. new "status" data format
Summary: 0 => imploded string of hotness.  Fixes T4689

Test Plan: for each spot i fixed, clicked link and it worked! (I did a grep for "/maniphest" to find these spots; 98% confident I got them all.)

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4689

Differential Revision: https://secure.phabricator.com/D8626
2014-03-28 12:59:09 -07:00
Chad Little
c3146abc8f Major timeline redesign
Summary: OMG We Have TOKENS

Test Plan: TOKENS, also UIExamples

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Differential Revision: https://secure.phabricator.com/D8624
2014-03-27 14:24:31 -07:00
Bob Trahan
de2da8355b Workboards - make priority changes less aggressive and generally better
Summary: Fixes T4641.

Test Plan: Dragged a "normal" task between "high" and "low" tasks and it stayed as "normal". Generally seems correct when playing around.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: mbishopim3, Beltran-rubo, epriestley, Korvin

Maniphest Tasks: T4641

Differential Revision: https://secure.phabricator.com/D8622
2014-03-27 10:50:54 -07:00
Bob Trahan
655ac9927f Workboards - add column detail page
Summary: followup to D8544. This ends up creating an editor + transactions to get the job done.

Test Plan: made a column - saw a nice created transaction. edited the name - saw a nice name edit. deleted the column - saw a deleted transaction, updated "deleted" ui, and hte action change to activate. "Activated" the column and saw a transaction and updated UI. Tried to delete a column with tasks in it and got an error.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8620
2014-03-26 14:40:47 -07:00
epriestley
1aad40b7bf Allow users to receive email about pushes via Herald
Summary:
Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries.

For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed.

Overall, this is basically the same as commit email, but more suitable for some workflows.

Test Plan:
Wrote some rules, then made a bunch of pushes. Got email like this:

{F134929}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8618
2014-03-26 13:51:15 -07:00
epriestley
75c47c6ae0 Provide an "event" page for push logs, which shows details on all events in a given push
Summary:
Ref T4677. This shows a more detailed view of an entire "git push", "hg push", or "svn commit".

This is mostly to give push summary emails a reasonable, stable URI to link to for T4677.

Test Plan:
  - Pushed into SVN, Git and Mercurial.
  - Viewed partial and imported event records.

{F134864}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8616
2014-03-26 13:51:09 -07:00
epriestley
a5f55d506f Provide a real object ("PhabricatorRepositoryPushEvent") to represent an entire push transaction
Summary:
Ref T4677. Currently, we record individual actions in a push as PhabricatorRepositoryPushLogs, but tie them together only loosely with a `transactionKey`.

Provide a real PushEvent object, and move some of the denormalized fields to it. This primarily just gives us more robust infrastructure for building, e.g., email about pushes, for T4677, since we can act on real PHIDs rather than passing awkward identifiers around.

Test Plan:
  - Performed migration.
  - Looked at database for consistency.
  - Browsed/queried push logs.
  - Pushed a bunch of stuff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8615
2014-03-26 13:51:06 -07:00
epriestley
c828160c22 Minor, fix a constant in PhabricatorRepositoryEditor
See <https://github.com/facebook/phabricator/pull/554>
2014-03-26 10:45:48 -07:00
James Rhodes
fc3b5ddce6 Prevent buildable list in Harbormaster from breaking when container or buildables are missing
Summary: Ref T1049.  I'm fair sure this is just a case of bad data in my local install, but we probably don't want the default page for Harbormaster to break when there's invalid / missing container or buildable handles on any of the builds.

Test Plan: Loaded the page, didn't get a crash due to null reference.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: demo, epriestley, Korvin

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8608
2014-03-25 17:35:49 -07:00
epriestley
e8e12910a7 Implement a "credential" standard custom field
Summary: Ref T4590. Ref T1049. This is primarily intended to support HTTP auth in Harbormaster.

Test Plan: Added a field, edited it, etc.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4590, T1049

Differential Revision: https://secure.phabricator.com/D8607
2014-03-25 16:13:27 -07:00
epriestley
6e3c17e6f9 Don't create invalid build steps while adding them
Summary:
Ref T1049. Currently, the "add" dialog lets you select a build step type, but then immediately creates one. If you "cancel" from the edit screen, you end up with an empty (and almost certainly invalid) build step.

Instead, don't create the step until it's valid.

Test Plan: Add Step -> Pick Type -> Add Step -> Cancel no longer creates empty step.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8605
2014-03-25 16:12:05 -07:00
epriestley
d6b937ca27 Allow external systems to send messages to build targets
Summary:
Ref T1049. Allows external systems to send a message to a build target. The primary intended use case is:

  - You make an HTTP request to Jenkins.
  - The build goes into a "waiting" state.
  - Later, Jenkins calls `harbormaster.sendmessage` to report that the target passed or failed.
  - The build continues as appropriate.

This is deceptively complicated because:

  - There are a lot of race concerns. We might get a message back from an external system before it even responds to the request we made. We want to make sure we process these messages no matter when we receive them.
  - These messages need to be sent to a build target (vs a build or buildable) because we'll get into trouble with parallelization later on otherwise (Jenkins is told to do 3 builds; we can't tell which ones failed or what overall state is unless the message are sent to targets).
  - I initially thought about implementing this as a separate "Wait for a response from an external system" build step. This gets a lot more complicated for users once we do parallelization, though. Particularly, in the case where you've told Jenkins to do 3 builds, the three "wait" steps need to know which target they're waiting for (and jenkins needs to know some unique identifier for each target). So this pretty much boils down to a more complicated, more error-prone version of using target PHIDs.

This makes the already-muddy Build UI a bit worse, but it needs a general clarity pass anyway (it's showing way too much uninteresting data, and should show a better summary of results instead).

Test Plan:
  - This doesn't really do anything interesting yet.
  - Used Conduit to send messages to build plans.
  - Viewed the messages on the build screen.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8604
2014-03-25 16:11:28 -07:00
epriestley
25f91567a7 Make various minor Harbormaster UI improvements
Summary: Ref T1049. Tweaks some of the UI and code to improve / clean it up a bit.

Test Plan: Ran build plans, browsed UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8603
2014-03-25 16:10:50 -07:00
epriestley
cec8d10731 Rename concrete Harbormaster step implementations
Summary: Ref T1049. For consistency, rename these to "Harbormaster...".

Test Plan: Ran migration, ran builds, everything still works fine.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8602
2014-03-25 16:09:51 -07:00
epriestley
281f06e281 Rename "BuildStepImplementation" to "HarbormasterBuildStepImplementation"
Summary: Ref T1049. D8588 already required custom code to change what it extends, so this is as good a time as we're going to get to move to more standard class name.

Test Plan: `arc liberate`; `arc lint`

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8601
2014-03-25 16:09:21 -07:00
epriestley
a246c85c6b Use ApplicationTransactions and CustomField to implement build steps
Summary:
Ref T1049. Fixes T4602. Moves all the funky field stuff to CustomField. Uses ApplicationTransactions to apply and record edits.

This makes "artifact" fields a little less nice (but still perfectly usable). With D8599, I think they're reasonable overall. We can improve this in the future.

All other field types are better (e.g., fixes weird bugs with "bool", fixes lots of weird behavior around required fields), and this gives us access to many new field types.

Test Plan:
Made a bunch of step edits. Here's an example:

{F133694}

Note that:

  - "Required" fields work correctly.
  - the transaction record is shown at the bottom of the page.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4602, T1049

Differential Revision: https://secure.phabricator.com/D8600
2014-03-25 16:08:40 -07:00
epriestley
72337dedaf Make Harbormaster input and output artifacts more explicit
Summary:
Ref T1049. In Harbormaster, build steps may have various inputs (like a host they should run on) and outputs (like a reference to an uploaded file).

  - Currently, inputs aren't defined anywhere (except implicitly at runtime).
    - Instead, define inputs explicitly.
  - Currently, outputs are defined in a way that loses information when misconfigured (the keys will collide).
    - Instead, define inputs and outputs so they work whether a step is configured correctly or not.
  - Currently, there's no simple way to see a step's inputs and outputs.
    - Add some UI for this.
  - Currently, reordering steps has some surprising side effects.
    - Instead of invalidating steps after reordering them, validate them at display time and warn the user.

Test Plan:
{F133679}
{F133680}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, chad

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8599
2014-03-25 16:02:34 -07:00
epriestley
5b74fa0a75 Make all build steps support variables
Summary: Ref T1049. This generally simplifies things. The steps which don't support variables generally don't make sense to support varaibles anyway.

Test Plan: Edited some steps.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8588
2014-03-25 16:02:07 -07:00
epriestley
ed2e12047a Downgrade accepts on "request changes", and make sticky accepts optional
Summary:
Fixes T3202. This fixes a couple of workflow issues:

  - Accepted Revision -> Request Review. Currently this stays "accepted" due to sticky rules being too aggressive, but should transition to "needs review".
  - Accepted Revision -> Plan Changes -> Request Review. Currently this stays "accepted". I think this behavior is correct, and have retained it. (In this case, you don't update the revision, you just "undo" your plan changes.) You can "Request Review" again to get back to "Needs Review".

Then implements a "sticky accept" switch:

  - When off, updates downgrade accepts.
  - When off, "request review" always downgrades accepts.

Test Plan:
  - Went through all (I think?) of the plan changes / request review / accept / update workflows, with sticky accept on and off.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3202

Differential Revision: https://secure.phabricator.com/D8614
2014-03-25 15:41:07 -07:00
epriestley
17dee98d32 Add a one-click "Scuttle Task" button to Maniphest
Summary: Fixes T4657. See that task for discussion of edge cases.

Test Plan: {F132941}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: chad, carl, epriestley

Maniphest Tasks: T4657

Differential Revision: https://secure.phabricator.com/D8590
2014-03-25 14:20:25 -07:00
epriestley
9ca86b69b7 Make Maniphest task statuses user configurable
Summary: Fixes T1812. Moves the internal configuration into public space and documents it.

Test Plan:
  - Tried to set it to some invalid stuff.
  - Set it to various valid things.
  - Browsed around, changed statuses, filtered statuses, viewed statuses, merged duplictes, examined transaction record, created tasks.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8585
2014-03-25 14:05:36 -07:00
epriestley
7713fb5d99 Add config validation for task status config
Summary: Ref T1812. This still doesn't expose configuration to the user, but adds validation for it.

Test Plan: Added a pile of unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8584
2014-03-25 14:04:51 -07:00
epriestley
07fdcde87e Provide viewer to CustomFields in ApplicationSearch
Summary: Fixes T4663.

Test Plan:
Added a custom field which accesses the viewer's username.

{F133249}

Reviewers: carl, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4663

Differential Revision: https://secure.phabricator.com/D8595
2014-03-25 14:02:18 -07:00
epriestley
2547a222d8 Guarantee that fields copied from diffs persist on revisions
Summary:
Fixes T4636. Currently, we copy fields from the diff to the revision during the external effect phase, but there's no guarantee that we persist the object after this phase.

(In practice, when Herald rules trigger they cause the object to persist on this install, which is why we don't see this issue.)

Instead, move the field copies to the internal phase, where persistence is guaranteed.

Also consolidate some of the diff loading.

Test Plan: Ran `arc diff`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: mbishopim3, epriestley

Maniphest Tasks: T4636

Differential Revision: https://secure.phabricator.com/D8610
2014-03-25 14:01:38 -07:00
epriestley
150a3adf2c Minor UI improvements for Harbormaster
Summary: Ref T1049. Makes some minor UI tweaks.

Test Plan: Looked at UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D8587
2014-03-25 13:59:43 -07:00
epriestley
62e3463ab7 Show primary email in approval queue interface
Summary:
Request from @csilvers. When approving users, the primary email address is useful for administrators.

(This queue is only accessible by administrators, so this doesn't expose email information in general.)

Test Plan: {F132912}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: shadowhand, csilvers, epriestley

Differential Revision: https://secure.phabricator.com/D8589
2014-03-25 13:59:01 -07:00
epriestley
0a76d82a7c Use string constants, not integer constants, to represent task status internally
Summary:
Ref T1812. I think integer constants are going to be confusing and error prone for users to interact with. For example, because we use 0-5, adding a second "open" status like "needs verification" without disrupting the existing statuses would require users to define a status with, e.g., constant `6`, but order it between constants `0` and `1`. And if they later remove statuses, they need to avoid reusing existing constants.

Instead, use more manageable string constants like "open", "resolved", etc.

We must migrate three tables:

  - The task table itself, to update task status.
  - The transaction table, to update historic status changes.
  - The saved query table, to update saved queries which specify status sets.

Test Plan:
  - Saved a query with complicated status filters.
  - Ran migrations.
  - Looked at the query, at existing tasks, and at task transactions.
  - Forced migrations to run again to verify idempotentcy/safety.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8583
2014-03-25 13:58:14 -07:00
epriestley
47d6d0bbad Drive Maniphest statuses with internal pseudo-configuration
Summary: Ref T1812. Without actually exposing configuration, this moves all status information into a config-like chunk of data which can later be exposed to human editors.

Test Plan:
  - Made a bunch of status changes.
  - Merged duplicates.
  - Created task.
  - Viewed feed, transaction record, etc.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8582
2014-03-25 13:56:45 -07:00
epriestley
fdc7b8672b Internalize Maniphest task status colors
Summary: Ref T1812. These are a bit fluff and don't make too much sense to make configurable, at least for now.

Test Plan: Grepped for external callers.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8581
2014-03-25 13:56:11 -07:00
epriestley
2a6d930480 Despecialize status handling in Maniphest Reports
Summary: Ref T1812. This is mega gross but Facts is too far away to do this right for now.

Test Plan:
bleh gross

Looked at reports, saw same data as before.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8580
2014-03-25 13:49:33 -07:00
epriestley
33bda2d590 Despecialize most task status handling
Summary: Ref T1812. Moves most specialized status handling into `ManiphestTaskStatus`. The only real missing case is reports.

Test Plan:
Browsed most of the affected interfaces. Changed task status:

{F132697}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8579
2014-03-25 13:47:42 -07:00
epriestley
750c872839 Update Google auth documentation to discuss "Google+ API" and new console URI
Summary: Fixes T4451. See also D8612.

Test Plan: Viewed panel and read text, saw it matched up with the new console.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4451

Differential Revision: https://secure.phabricator.com/D8613
2014-03-25 13:36:47 -07:00
epriestley
beccedb57c Make the "NOTE:" text bold and slightly darker
Summary: See screenshot. This does look like an improvement to me.

Test Plan: {F133255}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley, chad

Differential Revision: https://secure.phabricator.com/D8597
2014-03-22 18:06:46 -07:00
epriestley
9c872749a0 Restore psyduck to his proper place
Summary: Fixes T4665. The "attachable" logic was a little off after a recent change.

Test Plan: With and without a profile image, viewed a page.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4665

Differential Revision: https://secure.phabricator.com/D8594
2014-03-22 12:15:13 -07:00
Chad Little
8fb227d352 Update Remarkup Note Styles
Summary: Update notes, important, and warnings to look different than codeblocks.

Test Plan: test in diviner and legalpad

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad, avivey

Differential Revision: https://secure.phabricator.com/D8592
2014-03-21 21:42:39 -07:00
epriestley
ae03cb7fff Issue a proper 404 when trying to edit nonexistent events
Summary: We'll fatal a little later here when trying to call methods. 404 instead.

Test Plan: Visited `/calendar/event/edit/9999999/` or similar.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8591
2014-03-21 19:11:48 -07:00
epriestley
d8713f6f0b Make dialogs a little easier to use
Summary:
  - Dialog pages currently have no titles or crumbs, and look shoddy. Add titles and crumbs.
  - Dialog titles aren't always great for crumbs, add an optional "short title" for crumbs.
  - `AphrontDialogResponse` is pure boilerplate. Allow controllers to just return a `DialogView` instead and get the same effect.
  - Building dialogs requires a bit of boilerplate, and we generally construct them with no explicit `"action"`, which has some issues with T4593. Provide a convenience method to set the viewer and get a reasonable, explict submit URI.

Test Plan:
  - Viewed dialog on its own.
  - Viewed dialog as a dialog.

{F132353}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8577
2014-03-21 14:40:05 -07:00
epriestley
03c6bf0d09 Make Herald less ambitious about resolving repositories for revisions
Summary:
Fixes T4636. If a user manually deletes a "repository" setting from a revision, Herald attempts to resolve it. Instead, Herald should now just trust Differential. Generally, the new logic is:

  - When diffs are created, figure out repository information.
  - When revisions are updated, copy info from diffs.
  - Everywhere else, just trust the revision field.

Test Plan:
  - Created revisions.
  - Used Herald to dry-run revisions before and after a manual edit to remove the repository setting.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4636

Differential Revision: https://secure.phabricator.com/D8576
2014-03-21 14:39:56 -07:00
epriestley
70ed1ff7d0 Use standard UI kit on project member page
Summary: Fixes T4400. Removes very, very old "PhabricatorObjectListView", which was only used here.

Test Plan: {F132249}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley, chad

Maniphest Tasks: T4400

Differential Revision: https://secure.phabricator.com/D8574
2014-03-19 19:30:27 -07:00
epriestley
cebc7f4e8b Show profile pictures in subscribers dialog
Summary: Ref T4400. Also stops rendering "and 1 other" in subscriber lists, since it looks a bit silly in practice (we can just put the other subscriber there instead). Don't do the "and x others" until X is at least 2.

Test Plan: Viewed/clicked subscriber lists and transactions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4400

Differential Revision: https://secure.phabricator.com/D8573
2014-03-19 19:29:48 -07:00
epriestley
ef01aef45a Show user profile images on User list
Summary: Ref T4400. Same deal as projects. Tweaked the CSS a touch to make it look better in these views.

Test Plan: Viewed /people/.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley, chad

Maniphest Tasks: T4400

Differential Revision: https://secure.phabricator.com/D8571
2014-03-19 19:28:04 -07:00
epriestley
55d315a577 Show project profile images in project list view
Summary: Ref T4400. Adds project images to spice this UI up a bit.

Test Plan: Looked at list.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4400

Differential Revision: https://secure.phabricator.com/D8570
2014-03-19 19:27:37 -07:00
epriestley
221222ad7a Lock "security.require-https" from web edits
Summary: This can be used to lock yourself out of an instance, so prevent web edits.

Test Plan: Loaded page, wasn't web-editable.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8572
2014-03-19 19:27:04 -07:00
epriestley
3d639f5f98 Allow ObjectItemListView to show profile images
Summary: Ref T4400. Adds `setImageURI()` for object card/items.

Test Plan:
{F132229}

Also tested mobile.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley, chad

Maniphest Tasks: T4400

Differential Revision: https://secure.phabricator.com/D8569
2014-03-19 19:26:24 -07:00
epriestley
b8fafdbd90 Fix Passphrase cancel URI
Summary: The "Cancel" button on the "Edit Credential" interface doesn't go back to the "View Credential" interface for existing credentials.

Test Plan: Clicked "Cancel" on both "create" and "edit" workflows.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8568
2014-03-19 19:25:31 -07:00
epriestley
833b8a78d8 Reload subscribers before sending mail, etc
Summary: Fixes T4629. CCs added by Herald don't get added to the cached subscriber list. Just reload subscribers before sending mail to pick up effects.

Test Plan: Created an "always add X as CC" Herald rule for revisions, created a revision, saw them get initial mail.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: spicyj, epriestley

Maniphest Tasks: T4629

Differential Revision: https://secure.phabricator.com/D8565
2014-03-18 16:01:21 -07:00
epriestley
5721560663 Use modern UI for OAuthServer details page
Summary: Mostly just UI updates and policy enforcement. Improves error message when trying to authorize an already-authorized application.

Test Plan:
{F131584}

{F131585}

{F131586}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8564
2014-03-18 15:39:45 -07:00
epriestley
1534033664 Use ApplicationSearch in OAuthServer
Summary: Update the infrastructure and UI of the client list.

Test Plan: {F131570}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8563
2014-03-18 13:31:04 -07:00
epriestley
34c890b7e1 Use modern UI and policies in OAuth client editing
Summary:
Updates this stuff a bit:

  - Add a global create permission for OAuth applications. The primary goal is to reduce attack surface area by making it more difficult for an adversary to do anything which requires that they create and configure an OAuth application/client. Normal users shouldn't generally need to create applications, OAuth is complex, and doing things with user accounts is inherently somewhat administrative.
  - Use normal policies to check create and edit permissions, now that we have infrastructure for it.
  - Use modern UI kit.

Test Plan:
  - Created a client.
  - Edited a client.
  - Tried to create a client as a non-admin.
  - Tried to edit a client I don't own.

{F131511}

{F131512}

{F131513}

{F131514}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8562
2014-03-18 13:30:48 -07:00
epriestley
995a890565 Make OAuth client authorizations a Settings panel
Summary: This modernizes and simplifies OAuth client authorizations a bit, moving them to a settings panel similar to the "Sessions" panel.

Test Plan:
  - Viewed authorizations.
  - Revoked an authorization.
  - Created a test authorization.

{F131196}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8561
2014-03-18 13:28:19 -07:00
epriestley
8909f8ec59 Modernize OAuthServer PHIDs and Queries
Summary:
  - Modernize PHID types.
  - Implement `PhabricatorPolicyInterface`.
  - Make queries policy aware.

Test Plan: Browsed client and authorization lists.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: chad, epriestley

Differential Revision: https://secure.phabricator.com/D8560
2014-03-18 13:27:55 -07:00
epriestley
a2a4f4b3da Fix validation of Harbormaster HTTP methods
Summary: Precedence here was mucked up.

Test Plan: Plan with no explicit "method" now defaults to POST correctly.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8559
2014-03-18 12:05:14 -07:00
Bob Trahan
809e5a0389 Workboards - let users delete columns
Summary: Fixes T4408. I had to add a "status" to colum. I think we'll need this once we get fancier anyway but for now we have "active" and deleted.

Test Plan: deleted a column. noted reloaded workboard with all those tasks back in the default colun. loaded a task and saw the initial transaction had a "Disabled" icon next to the deleted workboard. also saw the new transaction back to the default column worked.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4408

Differential Revision: https://secure.phabricator.com/D8544
2014-03-18 10:40:31 -07:00
epriestley
1e8ed3e5ff Treat "request review" more like an update
Summary:
After "reject; plan changes; request review", revisions go back to "needs revision". Instead, they should remain in "needs review" (the reviewers need to review comments on the "request review", in the normal case). Generally, "request reivew" should act a lot like "update", just not actually change the diff.

To accomplish this, downgrade reviewers on "request review" to "rejected older", just like we would on an update.

Test Plan: Did "reject; plan; request", revision ended in "needs review". Rejected it into "needs revision"; updated it into "needs review".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: dctrwatson, epriestley

Differential Revision: https://secure.phabricator.com/D8558
2014-03-17 18:19:42 -07:00
epriestley
5b2887b69b Add "Date Updated" query fields for Maniphest
Summary:
Fixes T4637.

  - We already allow you to order by this column but don't have a key on it. Add one.
  - Expose UI for querying on ranges.

Test Plan:
  - Ran some queries, got reasonable-looking results and no table scans.

Reviewers: btrahan, bigo

Reviewed By: bigo

Subscribers: bigo, epriestley

Maniphest Tasks: T4637

Differential Revision: https://secure.phabricator.com/D8557
2014-03-17 15:53:07 -07:00
epriestley
ba8925a531 Support multiple LDAP filters in the Phabricator UI
Summary: Ref T3208. Not ready for prime time yet.

Test Plan: ldap T.T

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, frgtn, aran

Maniphest Tasks: T3208

Differential Revision: https://secure.phabricator.com/D8160
2014-03-17 15:08:30 -07:00
epriestley
7167a729bf Fail more gracefully when rendering transcripts if handle is missing
Summary: Fixes T4628. I can only partially reproduce the root cause here, but if transcript display rules aren't quite right we should just degrade here rather than fatalling. Transcripts are a messy business by any measure.

Test Plan: Sort-of-reproing transcript renders OK now.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4628

Differential Revision: https://secure.phabricator.com/D8554
2014-03-17 15:02:10 -07:00
epriestley
aea624118b Allow users to terminate login sessions
Summary:
This is partly a good feature, and partly should reduce false positives on HackerOne reporting things vaguely related to this.

Allow a user to terminate login sessions from the settings panel.

Test Plan:
  - Terminated a session.
  - Terminated all sessions.
  - Tried to terminate all sessions again.
  - Logged in with two browsers, terminated the other browser's session, reloaded, got kicked out.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8556
2014-03-17 15:02:01 -07:00
epriestley
38cc38eaf6 Modernize documentation links
Summary:
  - Point them at the new Diviner.
  - Make them a little less cumbersome to write.

Test Plan: Found almost all of these links in the UI and clicked them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8553
2014-03-17 15:01:31 -07:00
epriestley
039b8e43b9 Whitelist allowed editor protocols
Summary:
This is the other half of D8548. Specifically, the attack here was to set your own editor link to `javascript\n:...` and then you could XSS yourself. This isn't a hugely damaging attack, but we can be more certain by adding a whitelist here.

We already whitelist linkable protocols in remarkup (`uri.allowed-protocols`) in general.

Test Plan:
Tried to set and use valid/invalid editor URIs.

{F130883}

{F130884}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8551
2014-03-17 13:00:37 -07:00
epriestley
b3b0ef3647 Use viwer-based checks for application visibility when rendering home elements
Summary:
Fixes T4619. Currently, even if a viewer can't see Maniphest, they'll still see empty panels on the home page. These panels will always be empty so there's no real policy violation, but it's confusing.

Longer term, dashboards should fix this.

Test Plan: Viewed home page with a user with and without permissions on the apps.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4619

Differential Revision: https://secure.phabricator.com/D8545
2014-03-15 11:28:02 -07:00
epriestley
69eab4196d Use modern ApplicationTransactions "no effect" stuff in Maniphest
Summary: Fixes T912. This was very nearly working, it just needed a little tweaking on the last mile.

Test Plan:
Made updates with no effect, and updates with an effect. Made a no-effect update and posted just the comment part.

{F129037}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T912

Differential Revision: https://secure.phabricator.com/D8543
2014-03-14 15:13:51 -07:00
epriestley
559c0fe886 Tune cookie behaviors for 'phcid', 'phreg', etc
Summary:
Fixes T3471. Specific issues:

  - Add the ability to set a temporary cookie (expires when the browser closes).
  - We overwrote 'phcid' on every page load. This creates some issues with browser extensions. Instead, only write it if isn't set. To counterbalance this, make it temporary.
  - Make the 'next_uri' cookie temporary.
  - Make the 'phreg' cookie temporary.
  - Fix an issue where deleted cookies would persist after 302 (?) in some cases (this is/was 100% for me locally).

Test Plan:
  - Closed my browser, reopned it, verified temporary cookies were gone.
  - Logged in, authed, linked, logged out.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3471

Differential Revision: https://secure.phabricator.com/D8537
2014-03-14 14:33:31 -07:00
Bob Trahan
3ff9f5f48a Transactions - add "and X others" dialog support to application transactions
Summary: Fixes T4430. Basically does a little code massage from the new stuff in D8525 and application transactions to get this working. Adds a new controller to the subscriptions app to make rendering these pretty easy peasy.

Test Plan: Used my test task in D8525 to verify both add and rem versions of these dialogs worked correctly.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, chad, Korvin

Maniphest Tasks: T4430

Differential Revision: https://secure.phabricator.com/D8540
2014-03-14 14:27:45 -07:00
epriestley
6b4887ab22 Improve error and large file handling in thumbnailing
Summary:
Ref T2479, T4406. We should do a better job of (a) handling image processing errors and (b) declining to process large image files.

This fixes the worst of it, which is that users can upload huge GIFs with a large number of frames and hang a `convert` process for a long time, eating a CPU and a pile of memory.

This code is still pretty iffy and needs some more work. A near-term product goal for it is supporting 100x100 profile images.

Test Plan: Uploaded large and small GIFs, after setting the definition of "enormous" to be pretty small. Saw the small GIFs thumbnail into animated GIFs, and the large ones thumbnail into static images.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2479, T4406

Differential Revision: https://secure.phabricator.com/D8536
2014-03-14 11:53:49 -07:00
epriestley
48910537b9 Fix rendering of comments deleted by editing
Summary:
Fixes T4609. Steps are:

  - Make a comment.
  - Edit it.
  - Delete all the text.

We expect to see "This comment has been deleted." -- instead, things currently render goofy.

Root cause is that `hasComment()` means both "comment object exists" //and// "comment object is nonempty".

Test Plan: {F128862}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4609

Differential Revision: https://secure.phabricator.com/D8533
2014-03-14 11:53:30 -07:00
epriestley
838f781285 Add a robots.txt file to disallow /diffusion/
Summary:
Fixes T4610. Open to suggestions, etc., if there's anything I'm missing.

Also:

  - Moves these "system" endpoints into a real application.
  - Makes `isUnlisted()` work a little more consistently.

Test Plan: Accessed `/robots.txt`, `/status/` and `/debug/`.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4610

Differential Revision: https://secure.phabricator.com/D8532
2014-03-14 11:53:17 -07:00
epriestley
efcca310c2 In Differential mail, hide "summary" if empty and show "test plan" if nonempty
Summary:
Ref T4481. Summary is optional, but we currently always render it.

We previously rendered TEST PLAN. I wanted to see if anyone missed it. I miss it a little bit, and it sounds like @spicyj misses it. Restore it.

Test Plan:
  $ ./bin/mail show-outbound --id 15232
  ...
  BODY
  epriestley created this revision.
  epriestley added reviewers: The Bureaucracy, duck.
  epriestley added a subscriber: duck.

  TEST PLAN
    more j

  REVISION DETAIL
    http://local.aphront.com:8080/D1042

  AFFECTED FILES
    number_j.txt

  CHANGE DETAILS
    Index: number_j.txt
    ===================================================================
    --- number_j.txt
    +++ number_j.txt
    @@ -137,3 +137,4 @@
     j
     j
     j
    +j

  To: epriestley, duck, Sebastiangarcia, Ahmedsmoore, nathanhthomas, chewnicorn
  Cc: duck

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley, spicyj

Maniphest Tasks: T4481

Differential Revision: https://secure.phabricator.com/D8531
2014-03-14 11:53:06 -07:00
epriestley
f54bc8ae58 Add "Send an email" action to Herald for Maniphest
Summary: Fixes T4403. Supports the "send an email" action in Maniphest.

Test Plan: Wrote a "email duck" rule, then commented on a task and saw "duck" get an email.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4403

Differential Revision: https://secure.phabricator.com/D8529
2014-03-14 11:52:31 -07:00
epriestley
b168ee116b Fix "Send an email to" Herald rule for Differential
Summary: Herald returns a map of `phid => true`. This is unintuitive and should probably be cleaned up eventually.

Test Plan: With a "Send an email to" rule, updated a revision and saw no error in error log.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8527
2014-03-14 11:52:16 -07:00
epriestley
ab117c2baf Fix "arc diff --verbatim" when updating a revision
Summary: Fixes T4601. The "Differential Revision" field needs to be present in the "editable" version of the message so that `--verbatim` works correctly. Some day all of this might get rewritten to be a little easier to follow, maybe, but keep things working properly for now.

Test Plan: Used `arc diff`, `arc diff --edit`, `arc diff --verbatim`

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4601

Differential Revision: https://secure.phabricator.com/D8526
2014-03-14 11:50:22 -07:00
Bob Trahan
c7079b52a2 Subscriptions - make a dialog for massive subscription lists
Summary: Ref T4430. This just deploys it on the property lists. (Help on how to do translations better? I tried a more traditional pht('%s, %s, %s, and %d other(s)') but I think the string lookup assumes the %d comes as the second param or something?)

Test Plan: Made a Maniphest Task with a hojillion subscribers and noted the working dialogue. Also made a Pholio Mock with lots of subscribers and it worked.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin, chad

Maniphest Tasks: T4430

Differential Revision: https://secure.phabricator.com/D8525
2014-03-14 11:22:00 -07:00
Chad Little
3257372585 End Cap for Timeline
Summary: End-cap for timeline. Fixes T4438

Test Plan: Tested on a timeline with and without endcap.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin, chad, btrahan

Maniphest Tasks: T4438

Differential Revision: https://secure.phabricator.com/D8530
2014-03-14 08:51:50 -07:00
epriestley
6bda39aaad Fix a couple of minor OAuthServer things
Summary:
I was a bit hasty with this.

  - This should be uninstallable.
  - Provide a real description.
  - Choose a better title glyph (trident of neptune).

Test Plan: Poked around.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8534
2014-03-14 08:46:07 -07:00
epriestley
f7b1ed7221 Fix two registration errors for unusual provider emails
Summary:
See <https://github.com/facebook/phabricator/issues/541>.

  - If a provider returns the email `""` or `"0"`, we currently don't let the user edit it and thus don't let them register.
  - If a provider returns an invalid email like `"!!!"` (permitted by GitHub, e.g.), we show them a nonsense error message.

Instead:

  - Pretend we didn't get an address if we get an invalid address.
  - Test the address strictly against `null`.

Test Plan: Registered on Phabricator with my GitHub email set to `""` (empty string) and `"!!!"` (bang bang bang).

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8528
2014-03-13 19:03:12 -07:00
John Watson
8789f97867 Include objectPHID in feed.query text view
Summary: This way the type of story can be inferred.

Test Plan: requested feed.query with `view=>'text'`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8521
2014-03-13 17:56:25 -07:00
epriestley
1d30354223 Fix issue with ref/fixes in commit messages
Summary: Fixes T4600. If there's also a revision, the variable "$message" gets overwritten. groan~

Test Plan: Pushed a commit with "Fixes T123" and a revision, saw it parse on the first try.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: chrisbolt, aran, epriestley

Maniphest Tasks: T4600

Differential Revision: https://secure.phabricator.com/D8519
2014-03-13 17:47:11 -07:00
William R. Otte
29436dfe37 Added 'method' field to the HTTP request build step.
Summary:
This revision adds a 'method' field to the HTTP request harbormaster build step.  This allows the user to specify GET, POST, DELETE, and PUT (limited by the underlying wrapper phabricator uses for HTTP requests).  I'm not sure how much sense PUT makes, but oh well.

Existing plans shouldn't break, as if this field is an empty string, we default to POST, which is the old behavior.

Fixes T4604

Test Plan: 1) Verified that the empty string does, in fact, issue a POST request.  Changed the method to be GET and observed that the problem described in T4604 is resolved.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aran, epriestley

Maniphest Tasks: T4604

Differential Revision: https://secure.phabricator.com/D8520
2014-03-13 15:51:05 -07:00
Bob Trahan
e6118bcbaf Tweak application and maniphest editors to handle policy corner cases better
Summary:
Fixes T4362. If you have a default edit + view policy of "no one" things get weird when you try to create a task - basically its impossible.

Ergo, re-jigger how we do policy checks just a bit.

 - if its a new object, don't bother with the "can the $actor edit this thing by virtue of having can see / can edit priveleges?" That makes no sense on create.
 - add a hook so when doing the "will $actor still be able to edit this thing after all the edits" checks the object can be updated to its ultimate state. This matters for Maniphest as being the owner lets you do all sorts of stuff.

Test Plan:
- made a task with no one policy and assigned to no one - exception
- made a task with no one policy and assigned to me - success
 - made a comment on the task - success
 - reassigned the task to another user - exception
 - reassigned the task to another user and updated policies to "users" - success

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T4362

Differential Revision: https://secure.phabricator.com/D8508
2014-03-13 13:50:08 -07:00
epriestley
a9f38e55e5 Modernize Facebook OAuth instructions
Summary: Ref T4593. We have somewhat outdated instructions, and safer setup instructions are now available.

Test Plan: Read instructions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8518
2014-03-13 13:00:47 -07:00
epriestley
ae7324fd5b Fix an anchor redirect issue with OAuth server, plus modernize the application a bit
Summary:
Ref T4593. Via HackerOne. An attacker can use the anchor reattachment, combined with the Facebook token workflow, combined with redirection on OAuth errors to capture access tokens. The attack works roughly like this:

  - Create an OAuth application on Phabricator.
  - Set the domain to `evil.com`.
  - Grab the OAuth URI for it (something like `https://phabricator.com/oauthserver/auth/?redirect_uri=http://evil.com&...`).
  - Add an invalid `scope` parameter (`scope=xyz`).
  - Use //that// URI to build a Facebook OAuth URI (something like `https://facebook.com/oauth/?redirect_uri=http://phabricator.com/...&response_type=token`).
  - After the user authorizes the application on Facebook (or instantly if they've already authorized it), they're redirected to the OAuth server, which processes the request. Since this is the 'token' workflow, it has auth information in the URL anchor/fragment.
  - The OAuth server notices the `scope` error and 302's to the attacker's domain, preserving the anchor in most browsers through anchor reattachment.
  - The attacker reads the anchor in JS and can do client workflow stuff.

To fix this, I've made several general changes/modernizations:

  - Add a new application and make it beta. This is mostly cleanup, but also turns the server off for typical installs (it's not generally useful quite yet).
  - Add a "Console" page to make it easier to navigate.
  - Modernize some of the UI, since I was touching most of it anyways.

Then I've made specific security-focused changes:

  - In the web-based OAuth workflow, send back a human-readable page when errors occur. I //think// this is universally correct. Previously, humans would get a blob of JSON if they entered an invalid URI, etc. This type of response is correct for the companion endpoint ("ServerTokenController") since it's called by programs, but I believe not correct for this endpoint ("AuthController") since it's used by humans. Most of this is general cleanup (give humans human-readable errors instead of JSON blobs).
  - Never 302 off this endpoint automatically. Previously, a small set of errors (notably, bad `scope`) would cause a 302 with 'error'. This exposes us to anchor reattachment, and isn't generally helpful to anyone, since the requesting application did something wrong and even if it's prepared to handle the error, it can't really do anything better than we can.
  - The only time we'll 'error' back now from this workflow is if a user explicitly cancels the workflow. This isn't a 302, but a normal link (the cancel button), so the anchor is lost.
  - Even if the application is already approved, don't blindly 302. Instead, show the user a confirmation dialog with a 'continue' link. This is perhaps slightly less user-friendly than the straight redirect, but I think it's pretty reasonable in general, and it gives us a lot of protection against these classes of attack. This redirect is then through a link, not a 302, so the anchor is again detached.
  -

Test Plan: I attempted to hit everything I touched. See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8517
2014-03-13 12:59:10 -07:00
epriestley
969d0c3e8d Use "\z" instead of "$" to anchor validating regular expressions
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.

When we care about this, use `\z` instead, which matches "end of input" only.

This allowed registration of `"username\n"` and similar.

Test Plan:
  - Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
  - Fixed the ones where this seemed like it could have an impact.
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8516
2014-03-13 12:42:41 -07:00
epriestley
58877a9b6c Fix some collateral damage from SSH Keypair generation
Summary: There's no `addHiddenInput()` on the layout view. Instead, build it
from the form. See: <https://github.com/facebook/phabricator/issues/538>.

Auditors: btrahan
2014-03-13 07:31:45 -07:00
epriestley
f950985cfd Allow Passphrase to generate SSH keypairs and extact public keys from private keys
Summary:
Ref T4587.

  - Add an option to generate a keypair.
  - Add an option to view the public keys for existing keypairs.

Test Plan:
  - Generated keypairs.
  - Viewed public keys.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4587

Differential Revision: https://secure.phabricator.com/D8515
2014-03-12 18:58:25 -07:00
epriestley
44fc671b3f Add a "Generate Keypair" option on the SSH Keys panel
Summary: Ref T4587. Add an option to automatically generate a keypair, associate the public key, and save the private key.

Test Plan: Generated some keypairs. Hit error conditions, etc.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4587

Differential Revision: https://secure.phabricator.com/D8513
2014-03-12 18:17:11 -07:00
epriestley
d27cd5fb99 Disable Herald more aggressively when it's turned off for a repository
Summary:
Currently, disabling Herald only disables feed, notifications and email. Historically, audits didn't really create external effects so it made sense for Herald to only partially disable itself.

With the advent of Harbormaster/Build Plans, it makes more sense for Herald to just stop doing anything. When this option is disabled, stop all audit/build/publish/feed/email actions for the repository.

Test Plan: Ran `scripts/repository/reparse.php --herald`, etc.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8509
2014-03-12 18:16:50 -07:00
epriestley
8a409aa40f Fix parsing of "Reviewed By" field
Summary: In the Message parser, we read this field and expect to get an array of PHIDs out of it. Currently, we get a string. Instead, get an array of PHIDs.

Test Plan: Wrote a message like "Fixes Tnnn" with "Reviewed by: duck", and saw no more parse error during message parsing.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8510
2014-03-12 18:11:09 -07:00
epriestley
e068b04b5c Silence a warning about unknown fields in 'getcommitmessage'
Summary: Similar to D8491, some of my new exceptions are a bit too aggressive. See IRC. This one's hitting an edit workflow with 'revisionID' onboard somehow.

Test Plan: Not entirely sure how to hit this, but it won't throw anymore.

Reviewers: btrahan, dctrwatson, zeeg

Reviewed By: zeeg

Subscribers: zeeg, aran, epriestley

Differential Revision: https://secure.phabricator.com/D8514
2014-03-12 17:24:58 -07:00
Michael Peters
3f7f5a47ff Fix a small typo when creating a Herald rule
Summary: "Users who an edit" to "Users who can edit"

Test Plan: Verified that typo is gone after the change

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8511
2014-03-12 16:12:43 -07:00
epriestley
4173367f87 Add missing needReviewerAuthority() for differential.createcomment
Summary: For some actions (like accept) we need to load reviewer authority so we can figure out if the actor can act on behalf of project reviewers, etc.

Test Plan: Will make @dctrwatson do it.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: chad, aran, dctrwatson, epriestley

Differential Revision: https://secure.phabricator.com/D8505
2014-03-12 15:24:54 -07:00
epriestley
3b861ab741 Fix two transactional issues around revision status
Summary:
Two issues:

  - Herald is currently overwriting accepts and rejects with "blocking reviewer". Just stop it from doing that.
  - When you update an accepted revision, we put it back in "needs review", then return it to "accepted", generating an extra transaction. Instead, don't.

Test Plan:
  - Updated a revision with an accepting, herald-based blocking project reviewer. Reviewer was still accepting.
  - Updated an accepted revision, didn't get an extra transaction.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8506
2014-03-12 15:24:43 -07:00
epriestley
193e8a54fc Add "pusher is committer" to Herald as a pre-commit rule
Summary:
Fixes T4594. Also, allow "exists" / "does not exist" to be run against author/committer. This allows construction of rules like:

  - Committer identities must be authentic.
  - Committer identities must be resolvable.
  - Author identities must be resolvable.

Test Plan: Created some rules using these new rules and ran them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4594

Differential Revision: https://secure.phabricator.com/D8507
2014-03-12 15:24:33 -07:00
epriestley
08d013b391 Minor, fix parameter passed to HarbormasterBuildable::applyBuildPlans()
Summary: This should be a PHID, not an object.
2014-03-12 12:13:54 -07:00
epriestley
c0d433727c Modernize "revision update history" in Differential
Summary:
Ref T4585.

  - Use modern UI kit.
  - Make mobile-ish.
  - Fix a couple minor things.

Test Plan:
{F127155}

{F127156}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: aran, epriestley, chad

Maniphest Tasks: T4585

Differential Revision: https://secure.phabricator.com/D8504
2014-03-12 11:39:43 -07:00
epriestley
a4a4322d7a Add a "tags" field to Diffusion commit
Summary:
  - Fixes T4588.
  - See D8501.
  - Adds a "Tags" field for Herald commit emails.
  - Fixes a bug in `tagsquery` when filtering by commit name.
  - Make `tagsquery` just return nothing instead of fataling against Mercurial/Subversion.

Test Plan: Used `bin/repository/reparse.php --herald` to exercise this code.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4588

Differential Revision: https://secure.phabricator.com/D8502
2014-03-12 11:30:52 -07:00
epriestley
611f670720 Add CustomField to Diffusion and add a "branches" field
Summary:
Ref T4588. Request from @zeeg. Adds a "BRANCHES" field to commit emails, so the branches the commit appears on are shown.

I've implemented this with CustomField, but in a very light way.

Test Plan: Used `scripts/repository/reparse.php --herald` to generate mail, got a BRANCHES section where applicable.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley, zeeg

Maniphest Tasks: T4588

Differential Revision: https://secure.phabricator.com/D8501
2014-03-12 11:30:33 -07:00
epriestley
69399dfc2a Remove phpast.* Conduit methods
Summary:
Ref T4592. These were added with the intent of not requiring builds on Windows, but then we got builds on Windows working and they seem to be straightforward. See T4592 for most recent discussion.

Remove these methods because they aren't really practical for anything and increase attack surface area by giving adversaries access to `xhpast`, and generally bloat up the Conduit API. To my knowledge, nothing has ever called them.

(If an install somehow relies on these, they can drop them into `src/extensions/` to expose them again.)

Test Plan: Viewed conduit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4592

Differential Revision: https://secure.phabricator.com/D8500
2014-03-12 11:30:22 -07:00
epriestley
7176240717 Whitelist controllers which can receive a 'code' parameter
Summary:
Ref T4593. There are a variety of clever attacks against OAuth which involve changing the redirect URI to some other URI on the same domain which exhibits unexpected behavior in response to an OAuth request. The best approach to dealing with this is for providers to lock to a specific path and refuse to redirect elsewhere, but not all providers do this.

We haven't had any specific issues related to this, but the anchor issue in T4593 was only a step away.

To mitigate this in general, we can reject the OAuth2 `'code'` parameter on //every// page by default, and then whitelist it on the tiny number of controllers which should be able to receive it.

This is very coarse, kind of overkill, and has some fallout (we can't use `'code'` as a normal parameter in the application), but I think it's relatively well-contained and seems reasonable. A better approach might be to whitelist parameters on every controller (i.e., have each controller specify the parameters it can receive), but that would be a ton of work and probably cause a lot of false positives for a long time.

Since we don't use `'code'` normally anywhere (as far as I can tell), the coarseness of this approach seems reasonable.

Test Plan:
  - Logged in with OAuth.
  - Hit any other page with `?code=...` in the URL, got an exception.
  - Grepped for `'code'` and `"code"`, and examined each use to see if it was impacted.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8499
2014-03-12 11:30:04 -07:00
epriestley
9e3baacc95 Restore old "author can not be a reviewer" rule to Transactions
Summary:
This is a bit messy, but not tooo bad:

  - In general, stop the author from being added as a reviewer.
  - In the specific case that "self accept" is enabled, allow it. This is easier than trying to special case it.
  - When commandeering, we make the author a reviewer and make the actor the author, but these happen after validation. At validation time, it looks like we're making the author a reviewer. Just special case this.
  - Provide a slightly nicer message when trying to add yourself from `arc`. We hit the Transactions message anyway, but it's not formatted as cleanly.
  - Don't try to add the author via Herald.

Test Plan:
  - Edited a revision with author = reviewer, got stopped.
  - Commandeered revision.
  - Updated from `arc`.
  - Updated in general.
  - Fired a "add author as reviewer" Herald rule without things breaking.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8496
2014-03-12 06:04:30 -07:00
epriestley
95285aee50 When creating a new revision, include the summary in the initial email
Summary: This got dropped in the ApplicationTransactions stuff.

Test Plan: Created a new revision.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8495
2014-03-11 17:13:00 -07:00
epriestley
0b6c0c135f Default new reviewers to "added", and don't overwrite reviewer states when updating
Summary:
Fixes two issues with Differential:

  - New reviewers on initial diff were being created into a `null` state.
  - The `"="` edge update was overwriting accepted/rejected statuses. This could maybe be more nuanced in the long run, but I've just made it update correctly for now.

Test Plan:
  - Created and updated a revision, paying attention to reviewer statuses.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8494
2014-03-11 17:12:47 -07:00
epriestley
ad88ff28a1 Reject Phame domains which include a port number
Summary: Via HackerOne. This doesn't actually have any security impact as far as we can tell, but a researcher reported it since it seems suspicious. At a minimum, it could be confusing. Also improve some i18n stuff.

Test Plan: Hit all the error cases, then saved a valid custom domain.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8493
2014-03-11 15:53:15 -07:00
Bob Trahan
740757fd9b Diffusion - add ToC for readme files
Summary: see title. Fixes T4549.

Test Plan: made a readme that had some headers and observed a nice ToC

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T4549

Differential Revision: https://secure.phabricator.com/D8490
2014-03-11 15:52:16 -07:00
Bob Trahan
a1faac0a21 Phame - create conduit API to create posts
Summary:
nothing too crazy here. try to be smart about some defaults (i.e. phame title is optional and can be derived from title; post as not a draft by default; etc). Fixes T3695.

also do a little re-factoring to centralizing initializing new posts and turning posts into dictionaries. also change blogs => posts in another conduit method so it makes sense and stuff.

Test Plan: made some posts via conduit. testing trying to specify blogger, phame title, and isDraft, all worked nicely

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T3695

Differential Revision: https://secure.phabricator.com/D8485
2014-03-11 15:51:53 -07:00
Bob Trahan
46cf263e9d Fix a bug for updating diffs
Summary: Hit this issue in D8485. I think reviewedByPHID changes should appear in application transactions.

Test Plan: Would like to deploy this and try updating D8485 again.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8491
2014-03-11 15:36:03 -07:00
epriestley
0b15624c37 Fix two minor Differential issues
Summary: The JIRA field is currently always enabled. This isn't correct; it
should be disabled if there's no JIRA provider.

We also use the old set of reviewers to compute mail delivery. Instead, reload
the correct set of reviewers before moving on after finalizing transactions.
2014-03-11 13:50:13 -07:00
Neal Poole
8818252f52 [herald] Add support for Arcanist Project as a field for Differential revisions
Summary: Useful in cases where there is an Arcanist Project but not a repository tracked by Phabricator for a particular revision.

Test Plan: Created a new rule to flag Differential revisions with a particular Arcanist project, verified that it applied as expected via the test console to revisions with the project specified and with a different project specified.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8463
2014-03-11 13:15:14 -07:00
Michael Peters
8b6c86e27d Fix the script that saves lint for a repo into the database and updates diffusion.
Summary:
It appears a change to the way the configuration was loaded into ArcanistRepositoryAPI in rARCa2285b2b broke the save_lint script.
This updates the DiffusionLintSaveRunner to use the configuration correctly, allowing the linter to run

Test Plan: cd /your/project; ../../../path/to/phabricator/scripts/repository/save_lint.php

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8487
2014-03-11 13:07:45 -07:00
epriestley
2dbfb1d5fb Remove DifferentialComment
Summary: Ref T2222. Remove this; no more callsites.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8477
2014-03-11 13:02:33 -07:00
epriestley
592591e715 Clean up various pieces of dead/obsolete Differential code
Summary:
Ref T2222.

  - Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
  - Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
  - Moves Releeph churn field off `DifferentialCommentQuery`.
  - Removes dead code in `DifferentialRevisionViewController`.
  - Removes `DifferentialException` (no references).
  - Removes `DifferentialRevision->loadComments()` (no callsites).
  - Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
  - Removes `DifferentialCommentQuery` (all callsites updated).

Test Plan: Mostly a lot of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8476
2014-03-11 13:02:19 -07:00
epriestley
a49fec39be Move lint/unit test warning code forward to Transactions
Summary: Ref T2222. Makes the "lint/unit errors" warnings work again.

Test Plan: Viewed some revisions with and without these warnings.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8475
2014-03-11 13:02:18 -07:00
epriestley
20cc85878e Remove almost all old Differential field code
Summary: Ref T2222. The unit and lint fields still have one piece of functionality that I need to port, but everythign else is obsolete.

Test Plan: Lots of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8474
2014-03-11 13:02:16 -07:00
epriestley
3f67430f46 Use PhabricatorObjectListQuery in Herald worker
Summary: Ref T2222. Straightforward, just breaks a needless dependency.

Test Plan: Pushed and parsed a commit with "Auditors" in it.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8473
2014-03-11 13:02:14 -07:00
epriestley
7cd4e70ef2 Remove DifferentialFieldSelector
Summary: Ref T2222. Gets rid of DifferentialFieldSelector, favoring `differential.fields`.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8472
2014-03-11 13:02:13 -07:00
epriestley
40b471faea Move "close tasks on commit" code out of field specification stuff
Summary: Ref T2222. There's some magic here, just port it forward in a mostly-reasonable way. This could use some refinement eventually.

Test Plan: Pushed commits with "Fixes" and "Ref" language, used `reparse.php` to trigger the new code. Saw expected updates in Maniphest.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8471
2014-03-11 13:02:12 -07:00
epriestley
3910d0d5e1 Remove field selector on Diff view and Revision List View
Summary:
Ref T2222. This has some minor functionality regressions:

  - The plain diff page no longer shows unit/test status. I want to give diffs separate custom fields for this.
  - It was technically possible to shove more data on the list view, although this doensn't affect the default config.

Test Plan: Looked at list view, diff detail view. Grepped for changes.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8470
2014-03-11 13:02:10 -07:00
epriestley
48059265f3 Use CustomFields to power Conduit auxiliary dictionaries
Summary: Ref T2222. Moves this Conduit stuff over.

Test Plan: Made Conduit calls, saw data in results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8469
2014-03-11 13:02:09 -07:00
epriestley
77af6be803 Remove host/path and test plan enable/disable options
Summary: Ref T2222. These no longer have an effect, and are obsoleted by `differential.fields`.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8468
2014-03-11 13:02:07 -07:00
epriestley
9e8bbdb3a2 Port Differential mail features forward to transactions
Summary:
Ref T2222. Brings the major mail features (affected files, patches) forward.

This drops some of the minor integrations which just show object state (like "Maniphest Tasks") since I think they're not very important. I'll put them back if users miss them.

Test Plan: Sent mail with inline/attached patches.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8459
2014-03-11 13:02:06 -07:00
epriestley
50331016f7 Modernize commit message tips
Summary: Ref T2222. Fully modernizes these tips. No callsites remain for the old methods.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8457
2014-03-11 13:02:05 -07:00
epriestley
1df84168ef Remove DifferentialRevisionEditor
Summary: Ref T2222. This has no callsites and no functionality not present in the TransactionEditor.

Test Plan: awwyiss

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8456
2014-03-11 13:02:04 -07:00
epriestley
c68703fbcb Perform derived index updates in TransactionEditor
Summary: Ref T2222. We have two tables (one for hashes; one for paths) which were unevenly updated before. Now, update them consistently in the TransactionEditor.

Test Plan: Created a revision, saw it populate this information.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8455
2014-03-11 13:02:03 -07:00
epriestley
a19f49632f Remove willWriteRevision/didWriteRevision hooks
Summary:
Ref T2222. DifferentialRevisionEditor has no remaining callsites, but it has a bit of functionality which still needs to be ported forward. I'm going to rip it apart piece by piece.

This removes the willWriteRevision/didWriteRevision hooks. They are completely encapsulated by transactions now, except for a unique piece of branch/task logic, which I migrated forward.

Test Plan:
  - Lots of `grep`.
  - Created a new revision on branch `T25`, saw it associate with the task.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8454
2014-03-11 13:02:01 -07:00
epriestley
fbaa12440e Use DifferentialRevisionEditor in lipsum
Summary: Ref T2222.

Test Plan: Generated revisions with `bin/lipsum`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8453
2014-03-11 13:02:00 -07:00
epriestley
a5fbe921b7 Use CustomFields in differential.createrevision
Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.

This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.

Test Plan: Used `arc diff --create` to create several revisions with different data.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8452
2014-03-11 13:01:59 -07:00
epriestley
d8968755e9 Use CustomField for differential.updaterevision
Summary: Ref T2222. Ref T3794. Medium term goal is to remove `DifferentialRevisionEditor`. This removes one of two callsites.

Test Plan: Used `arc diff --edit` to repeatedly update a revision, making changes to various fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3794, T2222

Differential Revision: https://secure.phabricator.com/D8451
2014-03-11 13:01:58 -07:00
epriestley
6dd191a3c1 Allow configuration of Differential custom fields
Summary: Ref T2222. Ref T3886. This is a little early for general use, but the message parse/generate stuff requires CustomFields and FieldSpecifications to be closely aligned, so this provides at least a plausbile approach for any installs that run into trouble.

Test Plan: Viewed config; reordered fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222, T3886

Differential Revision: https://secure.phabricator.com/D8450
2014-03-11 13:01:57 -07:00
epriestley
ae3c1f7819 Perform commit message parsing and construction with new CustomFields
Summary: Ref T2222. Ref T3886. Converts parsing and construction of commit messages to be driven by CustomField.

Test Plan:
This is a huge, messy change. I've made an effort to test it exhasutively, but suspect I probably missed a few behaviors. Roughly:

  - Enumerted all current fields (fields implementing `shouldAppearOnCommitMessage()`) and tried to test them one by one.
  - Used `arc diff --edit` repeatedly to manipulate each field (this workflow hits both the parse and construct steps).
  - Used `arc amend --show` to examine construct output (this does not activate the "edit" mode).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8449
2014-03-11 13:01:55 -07:00
epriestley
966eb2ae26 Fail feed story renders individually, instead of in aggregate
Summary: When we fail to render a feed story because something is broken, just break that story, not the entire feed.

Test Plan: {F125898}

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8488
2014-03-10 18:22:24 -07:00
epriestley
a0f534b87c Minor improvements to Diviner layout
Summary:
  - Render functions as `func()` for consistency/clarity.
  - Sort articles first.
  - Sort case insensitively.
  - Label the "no group" symbols.

Test Plan: Regenerated and examined docs.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8480
2014-03-10 17:59:13 -07:00
Bob Trahan
8e41315238 Hovercard - add project images
Summary:
adds project images. Also fiddles with HTML + CSS just a bit so we have a "picture" column and a "details" column if a picture exists.

This keeps the details all in a nice column even if there are many details that end up being taller than the picture UI.

Fixes T3991.

Test Plan: looked at a task (no pic), project (pic w/ no details), and user (pic w/ many details) hovercard and all looked good on Chrome and Safari

Reviewers: epriestley, chad

CC: chad, Korvin, epriestley, aran

Maniphest Tasks: T3991

Differential Revision: https://secure.phabricator.com/D8483
2014-03-10 17:10:32 -07:00
epriestley
761b662283 Don't prefill "add email address" from GET
Summary: Via HackerOne. I don't think this is a security vulnerability, but it is inconsistent. There's no reason to prefill this, and I think the code was just lazy.

Test Plan:
  - Hit this page with `?email=xyz` in a GET request, no more prefill.
  - Looped the page with bad addresses, appropriate prefill.
  - Added an address.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8458
2014-03-10 16:21:47 -07:00
epriestley
042ab0ad9d Fix three minor edge case behaviors in Conpherence
Summary:
Couple of tweaks:

  - If a conpherence has no participants, we fail to `attachParticipants()`. This can happen if you leave a Conpherence as the last participant, then visit the URI again explicitly.
  - If you can't load any transactions (usually, because you don't have permission to view a thread's transactions), we try to attach `null` instead of `array()`. This can happen if you attempt to view a thread you don't have permission to see. A more general fix would be to tweak the load/filtering order, but I'm leaving that for another time since it's more involved and only gives us a small performance gain in unusual sitautions.
  - `initializeNewThread()` should be declared `static`.

Test Plan:
  - Viewed a thread with no participants, got proper policy error.
  - Viewed a thread I couldn't see, got proper policy error.
  - Grepped for `initializeNewThread()`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8467
2014-03-10 16:21:28 -07:00
epriestley
5854de8c1c Don't 302 to an external URI, even after CSRF POST
Summary:
Via HackerOne. This defuses an attack which allows users to steal OAuth tokens through a clever sequence of steps:

  - The attacker begins the OAuth workflow and copies the Facebook URL.
  - The attacker mutates the URL to use the JS/anchor workflow, and to redirect to `/phame/live/X/` instead of `/login/facebook:facebook.com/`, where `X` is the ID of some blog they control. Facebook isn't strict about paths, so this is allowed.
  - The blog has an external domain set (`blog.evil.com`), and the attacker controls that domain.
  - The user gets stopped on the "live" controller with credentials in the page anchor (`#access_token=...`) and a message ("This blog has moved...") in a dialog. They click "Continue", which POSTs a CSRF token.
  - When a user POSTs a `<form />` with no `action` attribute, the browser retains the page anchor. So visiting `/phame/live/8/#anchor` and clicking the "Continue" button POSTs you to a page with `#anchor` intact.
  - Some browsers (including Firefox and Chrome) retain the anchor after a 302 redirect.
  - The OAuth credentials are thus preserved when the user reaches `blog.evil.com`, and the attacker's site can read them.

This 302'ing after CSRF post is unusual in Phabricator and unique to Phame. It's not necessary -- instead, just use normal links, which drop anchors.

I'm going to pursue further steps to mitigate this class of attack more thoroughly:

  - Ideally, we should render forms with an explicit `action` attribute, but this might be a lot of work. I might render them with `#` if no action is provided. We never expect anchors to survive POST, and it's surprising to me that they do.
  - I'm going to blacklist OAuth parameters (like `access_token`) from appearing in GET on all pages except whitelisted pages (login pages). Although it's not important here, I think these could be captured from referrers in some cases. See also T4342.

Test Plan: Browsed all the affected Phame interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, arice

Differential Revision: https://secure.phabricator.com/D8481
2014-03-10 16:21:07 -07:00
epriestley
0a779b60a2 Exclude disabled (disapproved) users from count on People application on homepage
Summary:
The People application shows users awaiting approval, but incorrectly counts disabled users (i.e., users who were not approved).

Instead, count only non-disabled, non-approved users.

Test Plan: My homepage count dropped from 4 to 1, corresponding to the actual number of accounts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, spicyj

Differential Revision: https://secure.phabricator.com/D8486
2014-03-10 16:20:49 -07:00
Joshua Spence
e11adc4ad7 Added some additional assertion methods.
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.

This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.

Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8460
2014-03-08 19:16:21 -08:00
epriestley
06626205fe Minor, fix an issue with StoredCustomFields that serialize, like JIRA 2014-03-08 07:36:04 -08:00
Anirudh Sanjeev
44b41a94ae Add a note suggesting restarting daemons for feed.http-hooks
Summary:
I was trying to set up a http hook, but despite setting the config,
the endpoint wasn't getting a request. I was advised on IRC by balpert to
restart my daemons and it worked great after I did that.

Since this information isn't in the documentation, I am adding it to the
description of the option, so it helps the next person.

Test Plan: N/A

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran, spicyj

Differential Revision: https://secure.phabricator.com/D8447
2014-03-08 06:37:16 -08:00
epriestley
314edcabbb Fix Herald adapter construction for new revisions. Fixes T4572. 2014-03-08 06:35:41 -08:00
epriestley
76577df506 Extract textual object list parsing from Differential
Summary:
Ref T2222. Currently, Differential has a fairly hairy piece of logic to parse object lists, like `Reviewers: alincoln, htaft`. Extract, generalize, and cover this.

  - Some of the logic can be simplified with modern ObjectQuery stuff.
  - Make `@username` the formal monogram for users.
  - Make `list@domain.com` the formal monogram for mailing lists.
  - Add test coverage.

Test Plan:
  - Ran unit tests.
  - Called `differential.parsecommitmessage` with a bunch of real-world inputs and got sensible results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8445
2014-03-07 17:44:44 -08:00
epriestley
aff34077c5 Move Differential commit message parsing to a separate, tested class
Summary: Ref T2222. We have a hunk of logic that purely does text parsing here; separate it and get coverage on it.

Test Plan:
  - Ran new unit tests.
  - Used `differential.parsecommitmessage`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8444
2014-03-07 17:44:35 -08:00
epriestley
f25cce1e69 Remove DifferentialCommentEditor and DifferentialCommentMail
Summary: Ref T2222. These no longer have any callsites. Also got rid of a little bit of other code which also no longer has callsites.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8443
2014-03-07 17:44:19 -08:00
epriestley
1c51ed5940 Use TransactionEditor in differential.createcomment
Summary: Ref T2222. Update this callsite; pretty straightforward.

Test Plan: Used Conduit to take actions and saw their effects in Differential.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8442
2014-03-07 17:44:10 -08:00
epriestley
b04f706c0a Use TransactionEditor when closing revisions in response to commits
Summary: Ref T2222. When we discover a commit associated with a revision, close it using modern transactions.

Test Plan: {F123848}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8441
2014-03-07 17:43:58 -08:00
epriestley
458a28eed3 Truncate logSource in Harbormaster to the database column limit
Summary: This can be a command, which might be arbitrarily long, but the column is VARCHAR(255).

Test Plan: `grep`

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8446
2014-03-07 17:43:46 -08:00
epriestley
0488ce73c4 Add assertions to no-assertions tests in phabricator/
Summary: Ref T4570. Add trivial assertions to tests which fail-by-exploding so we can fail tests with no assertions.

Test Plan: Ran `arc unit --everything` with Arcanist patched to fail with no assertions.

Reviewers: leebyron, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4570

Differential Revision: https://secure.phabricator.com/D8436
2014-03-07 10:02:46 -08:00
Michael Peters
5e47f2a862 Adding author information to AuditListView
Summary:
Adding the Author to the home page and Audit overview page,
so that at a glance you can see who authored the commits
that need to be audited

Test Plan: View home page and audit overview page and see that author is shown

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8438
2014-03-07 08:40:35 -08:00
epriestley
49eaa9f8fe Use TransactionEditor in Differential mail handling
Summary: Ref T2222. Moves this instance of CommentEditor to TransactionEditor.

Test Plan: Used `bin/mail receive-test` to test receiving comment mail and action mail.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8427
2014-03-07 08:10:18 -08:00
epriestley
4dfd4944c4 Don't use CommentEditor in lipsum
Summary: Ref T2222. For now, I'm just dropping this rather than updating it since I'll need to come back here later for `DifferentialRevisionEditor` anyway, and no users rely on this functionality.

Test Plan: Static checks; this isn't user-facing.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8426
2014-03-07 08:10:07 -08:00
epriestley
75514cc0f7 Update differential.close to use DifferentialTransactionEditor
Summary: Ref T2222. Straightforward update to new stuff.

Test Plan:
  - Tried to close an uncloseable revision.
  - Closed a closeable revision.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8425
2014-03-07 08:09:59 -08:00
epriestley
3dc9afa28d Remove differential.markcommitted
Summary: Ref T2222. Primary goal is to remove this callsite for `DifferentialCommentEditor`, but rather than updating it I'm just nuking this method since it's been deprecated for more than a year (more than two years?)

Test Plan: Reloaded Conduit method list.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8424
2014-03-07 08:09:33 -08:00
epriestley
5801176edc When creating a new Phame blog post, check that the author has permission to post to the blog
Summary:
Via HackerOne. We're missing this permissions check, so you can sneak around it with URL editing right now.

I checked the other queries in this application and they seem OK.

Test Plan: Tried to post to a blog I had no permission to join.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8423
2014-03-06 14:06:07 -08:00
epriestley
270929dd6b Include Source Sans on-demand with Celerity
Summary:
Unwinds the mess I made in D8422 / D8430:

  - Remove `'fonts'`, since individual fonts can be included via Celerity now.
  - Include Source Sans from the local source when a document uses it as a fontkit.

Test Plan: Browsed Diviner, saw Source Sans.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8431
2014-03-06 11:39:48 -08:00
Peng Li
81d9935efe Allow parenthesis in author name
Summary: We have a dozen users who has `(...)` in their 'real name', like 'Jimmy (He) Zhang', and it's causing the diffusion file browser problems when blame is enabled. The parser does not expect those parenthesis and the lines of code will be empty if they were last touched by a user like that.

Test Plan: Try it

Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8429
2014-03-06 11:28:46 -08:00
epriestley
270916a26e Support WOFF files in Celerity and add Source Sans Pro
Summary:
  - Allow Celerity to map and serve WOFF files.
  - Add Source Sans Pro, Source Sans Pro Bold, and the corresponding LICENSE.
  - Add a `font-source-sans-pro` resource for the font.

Test Plan:
  - Changed body `font-face` to `'Source Sans Pro'`.
  - Added `require_celerity_resource('font-source-sans-pro')` in StandardPageView.

Works in Firefox/Chrome/Safari, at least:

{F123296}

{F123297}

{F123298}

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D8430
2014-03-06 11:28:24 -08:00
Bob Trahan
d08576ed4e Workboards - add task create + improve task placement wrt priority edits
Summary: Fixes T4553, T4407.

Test Plan: created tasks and they showed up in the proper column. edited task priority and they moved about sensically.

Reviewers: chad, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4553, T4407

Differential Revision: https://secure.phabricator.com/D8420
2014-03-05 18:40:28 -08:00
epriestley
001a9557ec Enable Diviner to use the markup cache. 2014-03-05 17:22:27 -08:00
epriestley
5b8e7d2de3 Minor, allow documentation to be accessed while logged out. 2014-03-05 17:19:40 -08:00
epriestley
f55168b08b Minor, actually show article titles in Diviner. 2014-03-05 16:48:29 -08:00
epriestley
867dae9414 Make Diviner's "advanced search" a little less derp
Summary: Ref T988. This layout got mucked up a while ago, restore it to some semblance of sanity and give it a couple of basic search options.

Test Plan: Searched for stuff. Woooo~~

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8419
2014-03-05 16:45:21 -08:00
epriestley
81d381f69d Give Diviner a non-legacy root controller
Summary: Ref T988. Instead of hard-coding the application landing page, make the Diviner root show books if any have been generated. Otherwise, show a helpful message about how to generate documentation locally.

Test Plan:
{F122723}

{F122724}

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8416
2014-03-05 13:07:50 -08:00
epriestley
d07fc70bbe Make bin/diviner generate with no arguments mean "generate everything"
Summary: Ref T988. This makes it easier to generate documentation.

Test Plan: Ran with and without `--book`. Examined CLI output.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8415
2014-03-05 13:00:50 -08:00
epriestley
fb52eda3d9 Allow Diviner books to have a "preface" section
Summary: Ref T988. This is primarily intended to let us add the "HEY! THIS ISN'T USER DOCUMENTATION" notices to the arcanist and libphutil technical docs.

Test Plan: Added some prefaces, generated docs, looked at them.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8410
2014-03-05 12:08:01 -08:00
epriestley
feac35a0fd Improve 404 behavior for new Diviner
Summary:
Ref T988. When the user clicks a link we haven't explicitly resolved before, we send them to the `/find/` endpoint, but currently just 404 if we can't find the relevant documentation.

Instead, display a more user-friendly error message, since we're probably going to have some of these. Also, make the page title much worse.

Test Plan: Hit a 404 via `/find/`, got a nicer page.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8408
2014-03-05 12:07:39 -08:00
epriestley
03abde0b25 Fix Diviner links to articles by title
Summary:
Ref T988. This fixes the biggest current problem with Diviner, which is dead links to articles.

In the new Diviner, articles can have both a "name" (derived from the file name, and used in the URI) and a "title" (optional, specified explicitly). For example, we have one document with the name "feedback" and the title "Give Feedback! Get Support!".

On disk, we want to use the name for the actual file where the text lives ("feedback.diviner"). We also want to use the name in the URI, to generate a clean URI and to allow us to retitle the document slightly without breaking links to it (for example, we renamed the "Backup" document to "Backups and Migrations").

However, when displaying the article we want to use the title.

Currently, you can //only// link to the name, not the title. This is inconvenient:

  - We have a bunch of existing docs which link to titles.
  - It's natural/intuitive to link to titles.
  - Linking to titles makes it easier/cheaper to generate documentation, because we don't need to be able to resolve things at render time.

To remedy this, allow links to target either names or titles. If we miss on a name query, we'll do a title query. This is implemented with a slug hash to allow approximately correct titles (wrong case/spacing/punctuation, e.g.) and sidestep all the UTF8/column length issues.

(In the long run, atom resolution should theoretically be more sophistiated than it is now, and we should do render-time lookups on at least some documents to catch bad links. However, this is fairly complicated and a relatively advanced feature, and I think allowing links to titles is desirable no matter what.)

Test Plan: The user documentation book now has valid links to articles when the titles and names differ.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8407
2014-03-05 12:07:26 -08:00
epriestley
2ceffadee7 Support Herald rules for new Differential edits
Summary:
Ref T2222. Ref T4484. See D8404 for discussion.

When a revision is updated with the new Editor, apply Herald rules. Additionally, apply them in a modern way which generates transactions.

Test Plan: {F122299}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Maniphest Tasks: T2222, T4484

Differential Revision: https://secure.phabricator.com/D8405
2014-03-05 12:07:13 -08:00
epriestley
84020a363f Let Herald activation depend on which transactions are being applied, and generate transactions
Summary:
Ref T2222. Ref T4484. This is a stepping stone to getting Herald supported in the new Differental code. Generally:

  - Instead of an Editor either supporting or not supporting Herald, let it choose based on transactions. Specifically, Differential only runs rules on revision creation and diff updates.
  - Optionally, allow an Editor to return some transactions to apply instead of having to apply everything itself. This lets us make it clear why changes happend in the transaction log, and share more code.
  - I updated only one transaction type (owners in Maniphest) since it was the easiest and cleanest to update and test. Everything else still works like it used to, it just won't generate a transaction record yet.
  - The transaction records are a touch rough, but we can clean them up later.

Test Plan: {F122282}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4484, T2222

Differential Revision: https://secure.phabricator.com/D8404
2014-03-05 12:06:59 -08:00
epriestley
4ef87eeac8 Add an explcit "Changes Planned" state for Differential
Summary: Ref T2222. Ref T4481. This fixes the issue where "Plan Changes" could immediately trigger a state change (e.g., back to accepted) because of state-based transitions out of the NEEDS_REVISION state.

Test Plan: Planned changes an "accepted" revision, it didn't immediately return to being accepted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222, T4481

Differential Revision: https://secure.phabricator.com/D8403
2014-03-05 10:47:47 -08:00
epriestley
9da6ec2081 Make updates of rejected revisions behave correctly again
Summary:
Ref T2222. Ref T4481. Specifically:

  - When a revision is updated, change all "Reject" reviewers to "Reject Prior".
  - Change status to "Needs Review".
  - Update the state logic to account for this properly.

Test Plan:
  - Created a revision as user A, with B as a reviewer.
  - Rejected as B.
  - Updated the revision as A.
  - Saw revision in "needs review" state, with B as a "Rejected Prior" reviewer.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4481, T2222

Differential Revision: https://secure.phabricator.com/D8402
2014-03-05 10:47:29 -08:00
epriestley
857e3aee83 Improve ApplicationTransaction behavior for poorly constructed transactions
Summary:
Ref T2222. Five very small improvements:

  - I hit this exception and it took a bit to understand which transaction was causing problems. Add an `Exception` subclass which does a better job of making the message debuggable.
  - The `oldValue` of a transaction may be `null`, legitimately (for example, changing the `repositoryPHID` for a revision from `null` to some valid PHID). Do a check to see if `setOldValue()` has been called, instead of a check for a `null` value.
  - Add an additional check for the other case (shouldn't have a value, but does).
  - When we're not generating a value, don't bother calling the code to generate it. The best case scenario is that it has no effect; any effect it might have (changing the value) is always wrong.
  - Maniphest didn't fall back to the parent correctly when computing this flag, so it got the wrong result for `CustomField` transactions.

Test Plan: Resolved the issue I was hitting more easily, made updates to a `null`-valued custom field, and applied other normal sorts of transactions successfully.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4557, T2222

Differential Revision: https://secure.phabricator.com/D8401
2014-03-05 10:44:21 -08:00
Chad Little
dd60f25232 Make "My Events" default on Calendar
Summary: Moves Browse to "View All" and makes "My Events" the default on Calendar.

Test Plan: Browse both pages.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8397
2014-03-05 08:24:45 -08:00
Bob Trahan
e78df59ced Maniphest Tasks + Project Boards - some polish
Summary:
Fixes T4550 by changing supportsFeed to shouldPublishFeedStory, so things can be more granular like that are with mail. Attempts to fix things generally too, filtering out xactions that have no business in feed, etc.

Also return an updated Task HTML representation on drag and drop moves, etc. This is important so if the priority changes you can see it reflected in the UI.

Test Plan: dragged tasks around. observed no feed stories on subpriority drags. observed feed stories and updated color bars on stories that changed priority

Reviewers: epriestley, chad

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4550

Differential Revision: https://secure.phabricator.com/D8399
2014-03-04 17:01:33 -08:00
epriestley
cdeea11fd3 Don't actually construct auth providers when checking for their existence
Summary:
A user reported this stack trace:

http://pastebin.com/6auGbZsE

...on this GitHub issue:

https://github.com/facebook/phabricator/issues/389#issuecomment-36612511

The problem is similar to the original report, but not identical. In this case, we're following a sequence of steps like:

  - Run setup checks.
    - Check for enabled providers, in order to raise "no providers configured yet" warning.
      - Try to generate login/redirect URIs.
  - Build the request.
  - Set the default base URI.
  - Run normal code.

Since we try to generate URIs before we provide a default, this fatals. Instead, don't try to build objects.

An alternative fix might be to try to set defaults earlier, but we depend on some config and on building the Request in order to be able to figure out if a request is HTTP or HTTPS right now. We could assume one, or guess, or use protocol-relative URIs (`///host.com`), but I think this fix is a little cleaner overall. If we keep hitting similar stuff, we could look into alternate fixes.

We could also set some kind of "setup mode" flag and make `getURI()` if it's called during setup mode to detect these during testing. I'd like to hit one more of these before doing that, though.

Test Plan: Reproduced the issue, applied the patch, verified this fixes it.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8395
2014-03-04 16:11:28 -08:00
epriestley
eec9507ba7 Link board names to board view in "changed column" stories
Summary: Fixes T4552.

Test Plan:
{F122106}

{F122107}

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4552

Differential Revision: https://secure.phabricator.com/D8396
2014-03-04 13:40:01 -08:00
Bob Trahan
28f3f8bf7b Board feed story - make the task a link too
Summary: looks better, more useful

Test Plan: looked better, was more useful when I observed my feed

Reviewers: chad, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8394
2014-03-04 12:05:42 -08:00
Bob Trahan
b49b913166 Project work boards - make editing tasks from workboards work
Summary: This wasn't working. Create a little JS handler and server-side support for returning the Task in the "project card" format.

Test Plan: Edited tasks from the board - they worked!

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran, chad

Differential Revision: https://secure.phabricator.com/D8392
2014-03-04 11:50:44 -08:00
Alex Wyler
5fabda2a6d Check for null, strictly, in maniphest.update param validation
Summary:
If the first non-null entry in the params array is falsey, the request bombs.

Something like {"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-on3xxsnaljmfn36d4b7a"}

Test Plan:
Before:

  echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
  {"error":"ERR-NO-EFFECT","errorMessage":"ERR-NO-EFFECT: Update has no effect.","response":null}

After:

  echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
  {"error":null,"errorMessage":null,"response":{"id":"279","phid":"PHID-TASK-lbwcq3pmur2c5fuqqhlx"...

Reviewers: garoevans, epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8391
2014-03-04 11:42:18 -08:00
Bob Trahan
d1e64e64ff Workboards - add transactions for column changes
Summary:
adds ManiphestTransaction::TYPE_PROJECT_COLUMN and makes it work. Had to clean up the Javascript ever so slightly as it was sending up the string "null" when it should just omit the data in these cases. Ref T4422.

NOTE: this is overall a bit buggy - e.g. move a task Foo from column A to top of column B; refresh; task Foo is at bottom of column B and should be at top of column B - but I plan on additional diff or three to clean this up.

Test Plan: dragged tasks around columns. clicked on those tasks and saw some nice transactions.

Reviewers: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4422

Differential Revision: https://secure.phabricator.com/D8366
2014-03-03 15:58:00 -08:00
epriestley
83206242c9 Clean up some Differential behaviors
Summary:
Ref T2222.

  - Restore mail tags for ApplicationTransactions mail.
  - Restore subject line verbs.
  - Denormalize line count and repository PHID.
  - Fix an issue with the mailgun adapter where headers weren't attached properly.

Test Plan: Sent some mail, verified it had correct subjects and tags.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8378
2014-03-03 08:36:40 -08:00
epriestley
e556d20577 Fix some issues where Conpherence would make to many draft requests
Summary:
A few minor fixes:

  - When we build a tag with `"meta" => null`, strip the attribute like we do for all other attributes. Previously, we would actually set the metadata to `null`. This happened with the Conpherence form.
  - Just respond to the draft request with an empty (but valid) response, instead of building a dialog.
  - `PhabricatorShapedRequest` is confusingly named and I should have caught this in review, but the basic shape of it is:
    - You make one object.
    - You call `trigger()` when stuff changes (e.g., a keystroke).
    - It manages making a small number of requests (e.g., one request after the user stops typing for a moment).
  - The way it was being used previously would incorrectly send a request for every keystroke.

I think I'm going to simplify `ShapedRequest` and merge it into some larger queue for T430.

Test Plan: Typed some text, no longer saw a flurry of requests. Reloaded page, still saw draft text.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran, chad

Differential Revision: https://secure.phabricator.com/D8380
2014-03-01 11:23:08 -08:00
epriestley
b383d2c338 Replace "Edit" controller with "EditPro" controller
Summary: Ref T2222. Remove the old controller and swap in the new ApplicationTransactions one.

Test Plan: Made a pile of edits.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8377
2014-02-28 16:49:30 -08:00
epriestley
bca0ad8fdd Make "EditPro" controller work with diff updates
Summary:
Ref T2222. Make the "EditPro" controller accommodate diff updates, and support the transaction type. This one is pretty straightforward.

Also make `revisionPHID` in the comments table nullable to fix the "Edit" action.

Test Plan:
  - Created new revision.
  - Updated revision.
  - Tried to do some invalid stuff.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8376
2014-02-28 16:49:22 -08:00
epriestley
8e32ab88c4 Minor, fix a missing parameter. 2014-02-28 15:25:49 -08:00
epriestley
11c1edfb74 Implement "words of power" against ApplicationTransactions
Summary:
Ref T2222. Differential has certain "words of power" (like `Ref T123` or `Depends on D345`) which should expand into a separate transaction when they appear anywhere in text.

Currently, they're respected in only some fields. I'm expanding them to work in any remarkup field, including comments and inline comments.

This partially generalizes transaction expansion/extraction in comments. Eventually, I'll probably implement some very soft sort of reference edge for T4036, maybe.

Test Plan: {F119368}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8369
2014-02-28 15:20:45 -08:00
epriestley
a3c1dcb928 Produce a more tailored checkout URI for Subversion
Summary:
For imported SVN repositories with an "Import Only" path, we produce a `/path/to/root/` URI, but should produce `/path/to/root/then/to/import/only/`.

As it is, the URI instructs the user to check out the whole repository.

Also, don't show the "Clone As" fragment in the URI for remote repositories, and prevent it from being edited for nonhosted repositories. This is generally more consistent with user expectation.

Test Plan:
  - Created a remote SVN repository with "Import Only", saw path include it.
  - Verified no "Clone As" options, no "Clone As" in URI.
  - Switched it to hosted, saw "Clone As" options appear and work properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, staticshock

Differential Revision: https://secure.phabricator.com/D8375
2014-02-28 13:04:41 -08:00
epriestley
d86bb086ca Reduce initial discovery from O(branches * commits) to O(commits)
Summary:
Fixes T4414. Currently, when we discover a new repository, we do something like this:

  foreach (branch) {
    foreach (commit on this branch) {
      do_something();
    }
  }

In cases where there are a lot of branches which mostly just branch `master`, this leads to us doing roughly `O(branches * commits)` work.

We have a `commitCache` to prevent this, but it has two problems:

  - It only fills out of the DB, and we do this whole thing before writing to the DB, which is the entire point.
  - It has a fixed size (2048) and on initial discovery we're usually dealing with way more commits than that.

Instead, also stop doing work if we hit a commit which is known already.

Test Plan:
  - Added `print` on the number of discovered refs and number of unique refs.
  - Ran `bin/repository discover --repair X` on a repo with several branches.
  - Before the patch, got 397 refs and 135 unique refs.
  - After the patch, got 135 refs and 135 unique refs.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4414

Differential Revision: https://secure.phabricator.com/D8374
2014-02-28 13:02:14 -08:00
epriestley
e28b78f5eb Fix two issues with ref discovery
Summary:
See IRC. I'm having trouble figuring out what's going on with b4taylor's report, but fix two possible issues:

  # The commit query is missing a `repositoryID`, which could cause issues if you import two copies of the same repository.
  # I think we may try to close commits on untracked branches right now, as long as they aren't excluded by other autoclose rules.

Test Plan: Ran `bin/repository refs` on a few repos.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, brennantaylor

Differential Revision: https://secure.phabricator.com/D8373
2014-02-28 12:56:18 -08:00
Chad Little
3a188de328 Align textarea in diff comment
Summary: Better aligns the text area when leaving an inline comment. Also, phts

Test Plan: reload page, view new padding.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8370
2014-02-28 08:40:02 -08:00
epriestley
024c331d2b Improve rendering of new-style Differential comments in feed/notifications
Summary: Ref T2222. This probably doesn't get everything, but should improve many of the newer transactions.

Test Plan: Looked at feed after making some edits.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8368
2014-02-27 15:16:32 -08:00
epriestley
647d52f248 Improve threading of new and old Differential mail
Summary: Ref T2222. This should help new mail thread properly with old mail.

Test Plan: Will push.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8367
2014-02-27 15:16:22 -08:00
epriestley
e62c7321c2 Automatically verify the setup account's email address
Summary: Although the defaults don't require a verified email address, it's easy to lock yourself out by accident by configuring `auth.require-email-verification` or `auth.email-domains` before setting up email. Just force-verify the initial/setup account's address.

Test Plan: Went through setup on a fresh install, saw address verify.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8365
2014-02-27 15:16:04 -08:00
epriestley
a5dbb1fd59 Make new JIRA Issues field editable
Summary: Ref T2222. This updates the new JIRA field to be editable.

Test Plan: Used `/editpro/` to edit associated JIRA issues.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8364
2014-02-27 15:15:48 -08:00
epriestley
ba7d67f917 Use "CommentPro" controller instead of "Comment" controller
Summary: Ref T2222. This will probabaly have a few rough edges too, but seems to work well.

Test Plan:
  - Made a bunch of comments while building this.
  - Made some new comments.
  - Verified that the Asana/JIRA integration is only a little bit janky, not completely broken.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8362
2014-02-27 11:06:55 -08:00
epriestley
62143b5455 Add modern Unit/Lint field support
Summary: Ref T2222. This is mostly copy/paste. No effect yet.

Test Plan: Looked at fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8360
2014-02-27 11:06:37 -08:00
epriestley
f6a13fd1c7 Use CustomField, not AuxiliaryField, to power RevisionView
Summary: Ref T2222. This will probably have some rough edges for a bit (e.g., weird cases I didn't remember or think of), but there's no change to the underlying data and we can easily revert if things get too messy.

Test Plan: Looked at a variety of revisions and saw sensible output.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8361
2014-02-27 11:06:14 -08:00
epriestley
0b4a6b8bee Add Branch and Arcanist Project CustomFields
Summary:
Ref T2222. These are pretty straightforward.

For these fields and a few others, the existing code shows the value for the "current/manual" diff (i.e., the diff selected in the diff selection table), not the "active" diff (i.e., the most recent diff attached to the revision). I'm going to drop that for now (always showing the most recent diff instead) and then reevaluate it once we're switched over. In 95% of cases these are the same, anyway.

Test Plan: Looked at fields; this diff changes nothing on its own.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8359
2014-02-27 11:05:48 -08:00
Bob Trahan
37b1b31638 Maniphest - move subpriority edits to be transaction powered
Summary:
...this was nice to do for boards, since this diff also starts calling this code in the board move case. The big trick is to use the new expandTransactions hook to expand the subpriority transaction into a priority transaction if pertinent. The other stuff is just about hiding these new subpriority extractions.

...also removes the "edit" UI from the default board since we can't actually edit anything and it thus is buggy.

Ref T4422. Next step is to move board edits into the editor with their own little transaction.

Test Plan: re-orded tasks on a maniphest query, reloaded, and noted re-order stuck.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4422

Differential Revision: https://secure.phabricator.com/D8358
2014-02-27 09:39:59 -08:00
epriestley
cce67caa0f Fix bad method call from CustomField migration
Fixes T4480.
2014-02-27 09:16:50 -08:00
epriestley
8297c2131c Implement more Differential fields on ApplicationTransaction/CustomField
Summary:
Ref T2222. Ref T3886. Gets the storage-based fields working.

This requires future changes to actually do anything, all this code is inactive.

Test Plan: {F118863}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8357
2014-02-26 16:53:42 -08:00
epriestley
cd7171ec6e Migrate old AuxiliaryField storage to modern CustomField storage
Summary: Ref T2222. Ref T3886. Differential has a legacy storage table for auxiliary fields; move the data to modern storage.

Test Plan:
  - Ran migration.
  - Verified fields still worked properly afterward (view, edit, etc).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8355
2014-02-26 16:52:30 -08:00
epriestley
ee2b6eebaa Implement detail views for many Differential fields on ApplicationTransactions
Summary:
Ref T2222. This isn't complete and doesn't change runtime behavior yet (the new fields are not glued to the interface), but implements many fields.

(The remaining fields have something weird going on with them, for the most part.)

Test Plan:
With additional changes, rendered most fields sensibly:

{F118834}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8354
2014-02-26 14:46:18 -08:00
epriestley
7ff2110655 Stop markup cache from hitting cache O(N^2) times
Summary:
The intent of getOrBuildEngine() is to save some boilerplate in cases where we're just using a standard engine, but it didn't get cached so we'd rebuilt it over and over again.

This was especially bad in Differential with a large number of inlines.

Test Plan: "Query" tab of services is no longer quite so ridiculous in Differential.

Reviewers: btrahan, dctrwatson

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8352
2014-02-26 13:17:28 -08:00
Joshua Spence
7f5b15c6fa Made some classes final.
Summary:
Currently, the linter raises `XHP29` warnings for these files because they are not abstract or final.

I guess there are two possibly solutions, either making the classes final or marking them as `@concrete-extensible`. Given that there are no subclasses of these classes in the `phabricator`, `arcanist` and `libphutil` repositories... I opted to declare the classes as final.

Test Plan:
The following linter warnings are gone:

```
>>> Lint for src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:

   Warning  (XHP29) Class Not abstract Or final
    This class is neither 'final' nor 'abstract', and does not have a
    docblock marking it '@concrete-extensible'.

               3 /**
               4  * @group aphront
               5  */
    >>>        6 class AphrontDefaultApplicationConfiguration
               7   extends AphrontApplicationConfiguration {
               8
               9   public function __construct() {

>>> Lint for src/applications/differential/mail/DifferentialReplyHandler.php:

   Warning  (XHP29) Class Not abstract Or final
    This class is neither 'final' nor 'abstract', and does not have a
    docblock marking it '@concrete-extensible'.

               1 <?php
               2
    >>>        3 class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
               4
               5   private $receivedMail;
               6

```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8347
2014-02-26 13:01:45 -08:00
Joshua Spence
67b32688c8 There should be no spaces before closing parenthesis in calls.
Summary: This currently raises a linter `XHP37` warning.

Test Plan: The file now lints okay.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8349
2014-02-26 12:49:33 -08:00
Joshua Spence
6270114767 Various linter fixes.
Summary:
- Removed trailing newlines.
- Added newline at EOF.
- Removed leading newlines.
- Trimmed trailing whitespace.
- Spelling fix.
- Added newline at EOF

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: hach-que, chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8344
2014-02-26 12:44:58 -08:00
epriestley
cd080b092e Use ApplicationTransactions/CustomField to power Differential global search
Summary:
Ref T2222. Ref T3886. Ref T418. A few changes:

  - CustomField can now index into global search.
  - Use CustomField fields instead of older custom fields for Differential global search. (This slightly breaks any custom fields which exist, but they are presumably very rare, and probably do not exist; this break is also very mild.)
  - Automatically perform CustomField and Subscribable indexing on applicable object types.

Test Plan: Used `bin/search index` to reindex a bunch of stuff, then searched for it. Debug-dumped abstract documents to inspect them.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418, T3886, T2222

Differential Revision: https://secure.phabricator.com/D8346
2014-02-26 11:18:06 -08:00
epriestley
bcf255e9c9 Require CSRF submission to verify email addresses
Summary: If an attacker somehow intercepts a verification URL for an email address, they can hypothetically CSRF the account owner into verifying it. What you'd do before (how do you get the link?) and after (why do you care that you tricked them into verifying) performing this attack is unclear, but in theory we should require a CSRF submission here; add one.

Test Plan: {F118691}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8351
2014-02-26 11:17:46 -08:00
epriestley
424ba2e588 Render inline comments in "Pro" mail
Summary: Ref T2222. This enriches mail a little bit to get these rendering pretty much like they do now.

Test Plan: {F118255}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8343
2014-02-25 15:29:10 -08:00
Bob Trahan
64d15c344e Passphrase - added "looked at secret" transaction.
Summary: Fixes T4376. Only thing I don't like in the current implementation is clicking "Done" doesn't refresh the page so you don't see the viewed secret transaction until you reload. Also made the textarea read-only as when I was playing with this for the first time I assumed I could also edit from the view secret side of things.

Test Plan: Viewed some secrets, saw some transactions.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4376

Differential Revision: https://secure.phabricator.com/D8345
2014-02-25 14:58:30 -08:00
Bob Trahan
9b0f906207 Fix calendar part 2
Summary: D8341 was a good start. However, I was looping through all the statuses each time, when I should only deal with a given status once. Instead, unset() a status from the list of statuses once we handled it. Also, delete the last old $key thing, which interfered with my chosen strategy.

Test Plan: made a two day event and verified it showed up in just those two days. (will push and test again just in case but this should be it)

Reviewers: epriestley, chad

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8342
2014-02-25 14:20:59 -08:00