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

6737 commits

Author SHA1 Message Date
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
ced70f6b32 Make install documentation more clear about Windows support
Summary: See D8549.

Test Plan: {F129985}

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8550
2014-03-16 13:00:20 -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
3bea0958dd Always link to "#5" (not "#comment-5") for "T123#5"
Summary:
Fixes T3976. Long ago, some applications used "#comment-5" instead of "#5" for transaction/comment anchors. Now everything (I think?) uses "#5"; this is the style used by ApplicationTransactions.

This might break some very old, explcit `T123#comment-5` links, or off-site links to the `comment-N` anchors, but all that stuff generally got renumbered when we migrated anyway and getting you to the right object is like 95% of the job.

Test Plan: Verified that `T123#5` now links to `#5`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3976

Differential Revision: https://secure.phabricator.com/D8542
2014-03-14 14:50:40 -07:00
epriestley
7be740cd30 Modernize CustomField documentation
Summary: Fixes T4102. Document all the new stuff that CustomField supports now, and all the applications you can use it with.

Test Plan: Generated and read documentation.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4102

Differential Revision: https://secure.phabricator.com/D8541
2014-03-14 14:50:27 -07:00
epriestley
8797c3ad0b Partly update the .arcconfig documentation
Summary: This needs more updates after .arclint is ready to use, but should fix most of the biggest issues.

Test Plan: Generated and read it.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8539
2014-03-14 14:33:53 -07:00
epriestley
60d8dc813e Document the security vulnerability reporting policy
Summary: Fixes T2791. I'm happy with HackerOne, so this pretty much just says "use HackerOne".

Test Plan:
{F128995}

  - Clicked all the links.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T2791

Differential Revision: https://secure.phabricator.com/D8538
2014-03-14 14:33:41 -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
epriestley
f1637961e7 Forbid "." and ".." in slugs
Summary: Fixes T4614. These don't do anything bad or dangerous, but generate unusable pages.

Test Plan:
  - Added and executed unit tests.
  - Tried to create pages like `/../`, `/begin/../end/`, etc.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: aran, epriestley

Maniphest Tasks: T4614

Differential Revision: https://secure.phabricator.com/D8535
2014-03-14 08:54:26 -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
a5f6e19dfd Make PhabricatorBotFeedNotificationHandler work with new PhabricatorApplicationTransactionFeedStory
Summary: Depends on D8521

Test Plan: ran with handler enabled and phabot posted stories

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8524
2014-03-13 18:20:05 -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
Chad Little
27485bc644 Add type=submit to AphrontDialogView
Summary: This is required for browsers <= IE7

Test Plan: Inspected HTML for type.

Reviewers: epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8503
2014-03-12 11:27:48 -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
873a4721b8 Divide Phabricator documentation into four books
Summary:
Ref T988. Divides documentation into four books:

  - User: Install, configure and use Phabricator.
  - Contrib: Develop and contribute to Phabricator.
  - Flavor: Worldly advice.
  - Generated: Generated technical documentation.

Test Plan: Generated the books and got sensible results. See screenshots.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8414
2014-03-05 13:00:24 -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
Eric Stern
3787a8d3db allow https.cabundle in IRC bot
Summary: Adds support for custom SSL certs in the IRC bot config, same as in .arcconfig

Test Plan: Bot wouldn't connect before. Added this code and corresponding line in bot config,  now it does.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8393
2014-03-04 11:54:54 -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
Roy Sindre Norangshol
fb87d0f491 (PhabricatorIRCProtocolAdapter.connect) Use recommended connection registration order from RFC{1459,2812}
http://tools.ietf.org/html/rfc1459.html#section-4.1 and http://tools.ietf.org/html/rfc2812#section-3.1
 mentions that client should send the PASS first, then the latter of NICK/USER.

This is tested and confirmed working with ngircd commit 485d0aec813db9966922f17aae044df2d82b0b67

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

Reviewed by: epriestley
2014-03-04 11:15:25 -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
05607aedf9 Normalize spacing around empty object lists
Summary: Adds an li for semantics, fixes spacing around error view in a phui-box or not

Test Plan: view a project with no tasks, perform a search with no data returned.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8371
2014-02-28 11:16:11 -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
cea58de0ba Fixed naming convention violations.
Summary: This file contains member variables prefixed with underscores. This violates the naming convention enforced by the linter.

Test Plan:
The linter warnings have disappeared. Example:
```
>>> Lint for src/infrastructure/storage/lisk/LiskDAO.php:

   Warning  (XHP9) Naming Conventions
    Follow naming conventions: class properties should be named using
    lowerCamelCase.

             183
             184   const COUNTER_TABLE_NAME          = 'lisk_counter';
             185
    >>>      186   private $__dirtyFields            = array();
             187   private $__missingFields          = array();
             188   private static $processIsolationLevel       = 0;
             189   private static $transactionIsolationLevel   = 0;
```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8348
2014-02-26 12:49:11 -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
Bob Trahan
b1e44239ba Fix calendar display on profile.
Summary:
...maybe anyway because I can't reproduce it live. This diff does two things that should help with bugginess though - uses $viewer rather than $user (...$user is who we are looking at...) *AND* upgrades a Conpherence util class to Calendar, and said util class has unit tests and came about from fixing a similar bug in Conpherence back in the day.

Wrote some comments in the util class because I think it has a tendency to trip people up. These comments are not partciularly good however.

Test Plan: viewed user profile - looked good. viewed conpherence - looked good. ran unit tests - they passed. (note I would also like to push this live and verify Chad's profile is fixed on secure.phabricator.com)

Reviewers: epriestley, chad

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8341
2014-02-25 13:43:31 -08:00
epriestley
06fc82a3ee Mark reviewers as "commented" when they leave a comment
Summary:
Ref T2222. This requires one new trick:

  - When merging edge transactions which both add/update an edge, the Editor gets to control how the edge data is merged.

Specifically, we pick the "strongest" state to keep, so "accept + comment" leaves you with an accept instead of a comment.

Test Plan: Accepted, commented on, and comment + accepted revisions. Added some debugging dumps to verify that the merging was getting hit and working correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8340
2014-02-25 12:37:11 -08:00
epriestley
a69cca9fbb Update overall revision status after reviewers change
Summary:
Ref T2222. This doesn't feel super clean, but doesn't feel too bad either.

Basically, Differential transactions can have secondary state-based effects (changing the overall revision status) when reviewers resign, are removed, accept, or reject revisions.

To deal with this in ApplicationTransactions, I did this:

  - `applyFinalEffects()` can now alter the transaction set (notably, add new ones). This mostly matters for email, notifications and feed.
  - In Differential, check for an overall revision state transition in `applyFinalEffects()` (e.g., your reject moving the revision to a rejected state).
  - I'm only writing the transaction if the transition is implied and indirect.
    - For example, if you "Plan Changes", that action changes the state on its own so there's no implicit state change transaction added.

The transactions themselves are kind of fluff, but it seems useful to keep a record of when state changes occurred in the transaction log. If people complain we can hide/remove them.

Test Plan: {F118143}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8339
2014-02-25 12:36:49 -08:00
epriestley
ca8c2c2d11 Implement Accept/Reject in ApplicationTransactions, approximately
Summary: Ref T2222. This mostly makes Accept/Reject work. The big missing piece is that overall revision status does not yet update properly. I need to think about how I want that to work a little bit more.

Test Plan: Accepted and rejected some stuff.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8333
2014-02-25 12:36:39 -08:00
epriestley
d07292df7f Implement Commandeer action in ApplicationTransactions
Summary: Ref T2222. Implements Commandeer on the new stuff.

Test Plan:
{F117768}

{F117769}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8331
2014-02-25 12:36:24 -08:00
epriestley
0f0673b9e5 Remove "dateCommitted" field from DifferentialRevision
Summary: Ref T2222. This is obsolete and no longer used. We could deduce it from transactions or commits in modern Phabricator if we wanted it. We may implement a more general mechanism for T4434.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8330
2014-02-25 12:36:14 -08:00
epriestley
9292433dae Implement "Resign" action against ApplicationTransactions
Summary:
Ref T2222. This introduces two small new concepts:

  - `expandTransactions()`: allows a transaction to expand into several transactions. For example, "resign" adds a "remove reviewers" transaction.
    - We have some other cases which could use this, but currently hard-code things outside of the `Editor`.
      - One example is that in Maniphest, closing a task implies claiming it if it is unowned.
  - `setIgnoreOnNoEffect()`: The whole Editor can be set to continue or stop if any transactions have no effect, but this allows the behavior to be refined at the individual transaction level. This is primarily to make the UX less confusing, so the user gets only a single relevant error instead of one for each expanded transaction.

Otherwise, this is pretty straightforward.

Test Plan:
Rigged comment form to use SavePro controller, enabled resign action, then tried to resign from a bunch of stuff.

{F117743}

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8328
2014-02-25 12:36:02 -08:00
epriestley
72a062d802 Truncate Conduit 'clientDescription' so we don't overflow the column
Summary: We store the `arc` commandline in this 255-character column, but it can be more than 255-characters long. If it's huge, truncate it.

Test Plan:
Executed:

  arc list --conduit-uri=http://local.aphront.com:8080/ --conduit-version 6.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

Works fine after this patch.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8327
2014-02-25 12:35:03 -08:00
Chad Little
c79623f756 Add allday, multiday logic to AphrontEventView
Summary: Hardens up the logic for DST and makes them easier to access elsewhere.

Test Plan: view sample events, all day and multiday, in my sandbox

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8332
2014-02-24 20:21:28 -08:00
epriestley
d19e2155be Reuse common getUser() / setUser() in PHUICalendarListView
Summary:
These methods already exist in AphrontView. The redefinition can sometimes cause this warning:

  [2014-02-24 18:27:48] ERROR 2048: Declaration of PHUICalendarListView::setUser() should be compatible with AphrontView::setUser(PhabricatorUser $user) at [/INSECURE/devtools/phabricator/src/view/phui/calendar/PHUICalendarListView.php:138]

Test Plan: Viewed calendar on profile.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8329
2014-02-24 18:55:19 -08:00
epriestley
4eadcf975a Implement most Differential actions against ApplicationTransactions
Summary: Ref T2222. Implements the simpler actions (abandon, reclaim, close, reopen, plan changes, request review) in a transactional way with validation and effect checks.

Test Plan:
  - Rigged submissions to point at the Pro controller.
  - Rigged dropdown to have all these options all the time.
  - Tried to apply about 20-30 of these operations to various revisions and always got the expected result (success, error, or no-op).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8307
2014-02-24 15:57:49 -08:00
epriestley
d48a88d4cd Add inline comment support to "Pro" comment save controller
Summary: Ref T2222. Makes the "pro" controller work with inlines.

Test Plan: Added a bunch of inlines and saved them with the "pro" controller.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8306
2014-02-24 15:57:35 -08:00
epriestley
c7d208fda1 Add a "Pro" version of the Differential comment save controller
Summary: Ref T2222. Adds a mostly-functional "Pro" comment controller. This does the core stuff, but does not yet do actions (accept, reject, etc.) or inline comments.

Test Plan: Changed the `if (false)` to an `if (true)`, then made some comments, etc. This is normally unreachable.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8304
2014-02-24 15:57:26 -08:00
epriestley
3a8eb28a07 Send basic email from new ApplicationTransaction editor
Summary: Ref T2222. Adds basic support for email.

Test Plan: Received an email via `/editpro/`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8303
2014-02-24 15:57:16 -08:00
epriestley
763a049f74 Possibly fix issue where refs engine finds empty refs?
Summary:
@mbishopim3 reported an issue in IRC:

> mbishopim3: epriestley: "Error updating working copy: Commit "" has not been discovered yet! Run discovery before updating refs." any ideas?

I can't reproduce it and it went away for him, but one theory is that we're getting here and git/hg are spitting out nothing, which we incorrectly parse as `array("")` when we intend `array()`.

Test Plan:
Pushed some new commits, ran `bin/repositoy refs X`, got expected results.

I can't actually reproduce the bug, but this might fix it and appears to make the code more correct.

Reviewers: btrahan

Reviewed By: btrahan

CC: mbishopim3, aran

Differential Revision: https://secure.phabricator.com/D8326
2014-02-24 14:39:44 -08:00
epriestley
ca851c4d85 Improve Windows install instructions
Summary: This advice is clearer if we also tell you to remove the comment.

Test Plan: Reading adventure!

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8325
2014-02-24 12:20:53 -08:00
epriestley
a31a7809b0 Fix a type issue with FormView juggling
Summary:
When you click the pencil icon in the Maniphest task list, we currently fatal:

  Argument 1 passed to PhabricatorCustomFieldList::appendFieldsToForm() must be an instance of AphrontFormView, instance of PHUIFormLayoutView given, called in /core/lib/phabricator/src/applications/maniphest/controller/ManiphestTaskEditController.php on line 576 and defined

This is because we build an `AphrontFormView` noramlly, but a `PHUIFormLayoutView` for dialogs, since they don't take a full form (they render their own form tag).

Instead, always build an `AphrontFormView` and just pull the `PHUIFormLayoutView` out of it when we're ready to put it in a dialog. This means `$form` is always the same type of object, and is generally better and makes more sense.

Test Plan: Clicked pencil edit icon in Maniphest task list.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran, carl

Differential Revision: https://secure.phabricator.com/D8324
2014-02-24 12:20:49 -08:00
epriestley
5f95f2b6d5 Fix variable issue on Conpherence draft saving pathway
Summary:
When we save a Conpherence draft, the draft part works fine but `$xactions` never gets defined, so this gets hit a little later on:

  [24-Feb-2014 11:46:10] WARNING: [pool www] child 82805 said into stderr: "NOTICE: PHP message: [2014-02-24 11:46:10] EXCEPTION: (RuntimeException) Undefined variable: xactions at [/INSECURE/devtools/libphutil/src/error/PhutilErrorHandler.php:211]"
  [24-Feb-2014 11:46:10] WARNING: [pool www] child 82805 said into stderr: "NOTICE: PHP message:   #0 PhutilErrorHandler::handleError(8, Undefined variable: xactions, /INSECURE/devtools/phabricator/src/applications/conpherence/controller/ConpherenceUpdateController.php, 122, Array of size 13 starting with: { request => Object AphrontRequest }) called at [/INSECURE/devtools/phabricator/src/applications/conpherence/controller/ConpherenceUpdateController.php:122]"
  [24-Feb-2014 11:46:10] WARNING: [pool www] child 82805 said into stderr: "NOTICE: PHP message:   #1 ConpherenceUpdateController::processRequest() called at [/INSECURE/devtools/phabricator/webroot/index.php:87]"

Instead, define `$xactions`.

Test Plan:
  - Type into Conpherence while tailing the error log.
  - After patch, clean error log.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8323
2014-02-24 11:56:04 -08:00
epriestley
14627ad65b Fix an incorrectly spelled call on the registration error pathway
Summary: If you copy the registration URL, then register, then load the URL again while logged out (i.e., attempt to reuse the registration URL), we try to show you a tailored error message. However, this call is not correct so we show you a not-so tailored exception instead.

Test Plan:
  - Get to the registration screen.
  - Save URL.
  - Complete registration.
  - Log out.
  - Return to saved URL.

Previously, exception. Now, readable error.

{F117585}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8322
2014-02-24 11:45:28 -08:00
Chad Little
396e8ba82c Calendar upgrades
Summary:
Does a handful of things to make Calendar significantly more useful

- Enabled overlapping events
- Profile has a 'week view' of the user
- Profile has a 'month view' of the users
- Multiple users on a calendar are color coded
- Browse view slightly more useful

This stops short of implementing the new 'home' view on Calendar, mostly this is a big step though to make that happen next.

Test Plan: Make lots of events on diffent users.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T2897, T4375

Differential Revision: https://secure.phabricator.com/D8317
2014-02-24 10:04:23 -08:00
epriestley
e7254944ec Use JSON, not PHP serialization, for XHProf profiles. 2014-02-24 04:16:52 -08:00
epriestley
f49470f9bf Add rough validation on email addresses
Summary: Put a very rough filter on what we'll accept as an email address. We can expand this if anyone is actually using local delivery or other weird things. This is mostly to avoid a theoretical case where some input is parsed differently by `PhutilAddressParser` and the actual mail adapter, in some subtle hypothetical way. This should give us only "reasonable" email addresses which parsers would be hard-pressed to trip up on.

Test Plan: Added and executed unit tests. Tried to add silly emails. Added valid emails.

Reviewers: btrahan, arice

Reviewed By: arice

CC: arice, chad, aran

Differential Revision: https://secure.phabricator.com/D8320
2014-02-23 17:31:46 -08:00
epriestley
a566ae3730 Require a CSRF code for Twitter and JIRA (OAuth 1) logins
Summary:
OAuth1 doesn't have anything like the `state` parameter, and I overlooked that we need to shove one in there somewhere. Append it to the callback URI. This functions like `state` in OAuth2.

Without this, an attacker can trick a user into logging into Phabricator with an account the attacker controls.

Test Plan:
  - Logged in with JIRA.
  - Logged in with Twitter.
  - Logged in with Facebook (an OAuth2 provider).
  - Linked a Twitter account.
  - Linked a Facebook account.
  - Jiggered codes in URIs and verified that I got the exceptions I expected.

Reviewers: btrahan, arice

Reviewed By: arice

CC: arice, chad, aran

Differential Revision: https://secure.phabricator.com/D8318
2014-02-23 16:39:24 -08:00
epriestley
438915032a Minor, mark SERIALIZATION_PHP fields as BINARY in Lisk 2014-02-23 16:35:51 -08:00
epriestley
a298a79bda Convert Phabricator to handle "%s" / "%B" properly
Summary:
Ref T1191. I believe we only have three meaningful binary fields across all applications:

  - The general cache may contain gzipped content.
  - The file storage blob may contain arbitrary binary content.
  - The Passphrase secret can store arbitrary binary data (although it currently never does).

This adds Lisk config for binary fields, and uses `%B` where necessary.

Test Plan:
  - Added and executed unit tests.
  - Forced file uploads to use MySQL, uploaded binaries.
  - Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
  - Tried to register with an account containing a G-Clef, and was stopped before the insert.

Reviewers: btrahan, arice

Reviewed By: arice

CC: arice, chad, aran

Maniphest Tasks: T1191

Differential Revision: https://secure.phabricator.com/D8316
2014-02-23 16:20:46 -08:00
epriestley
70b008d18d Add test coverage that our definition of BMP agrees with MySQL
Summary:
Ref T1191. Test that MySQL's rules match those of `phutil_is_utf8_with_only_bmp_characters()`:

  - Build a string with //every// character that we consider to be a BMP character.
  - Write it into MySQL.
  - Read it back out.
  - Make sure MySQL didn't truncate it.

Test Plan: Ran unit test. This test runs pretty quickly (50ms), the string with every character isn't all that enormous.

Reviewers: btrahan, arice

Reviewed By: arice

CC: chad, arice, aran

Maniphest Tasks: T1191

Differential Revision: https://secure.phabricator.com/D8314
2014-02-23 16:20:38 -08:00
epriestley
bbf40146fb Recommend STRICT_ALL_TABLES for every install, not just development installs
Summary:
See D8308. Enabling STRICT_ALL_TABLES prevents this entire class of error, by fataling on truncation instead of truncating. We never want truncation; it is always bad and sometimes extremely bad.

We've recommended this mode for developer installs for a long time, and some users run with it enabled, so it's very unlikely to cause any issues (I've had it enabled locally for at least 6-8 months, I think).

Test Plan:
  - Disabled mode.
  - Saw warning.
  - Enabled mode.
  - No warning.

{F117040}

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran, arice

Differential Revision: https://secure.phabricator.com/D8309
2014-02-23 10:59:59 -08:00
epriestley
7cf0358dda Disallow email addresses which will overflow MySQL storage
Summary:
Via HackerOne. An attacker can bypass `auth.email-domains` by registering with an email like:

  aaaaa...aaaaa@evil.com@company.com

We'll validate the full string, then insert it into the database where it will be truncated, removing the `@company.com` part. Then we'll send an email to `@evil.com`.

Instead, reject email addresses which won't fit in the table.

`STRICT_ALL_TABLES` stops this attack, I'm going to add a setup warning encouraging it.

Test Plan:
  - Set `auth.email-domains` to `@company.com`.
  - Registered with `aaa...aaa@evil.com@company.com`. Previously this worked, now it is rejected.
  - Did a valid registration.
  - Tried to add `aaa...aaaa@evil.com@company.com` as an email address. Previously this worked, now it is rejected.
  - Did a valid email add.
  - Added and executed unit tests.

Reviewers: btrahan, arice

Reviewed By: arice

CC: aran, chad

Differential Revision: https://secure.phabricator.com/D8308
2014-02-23 10:19:35 -08:00
Chad Little
a4d4bf8196 Add ObjectBox around Diffusion Binary Files
Summary: Add in more ObjectBoxes

Test Plan: Test aphlict.swf, see new menu and button to download.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8305
2014-02-22 14:08:04 -08:00
Chad Little
8662b27f89 Move raw file download icon to file box in Diffusion
Summary: For images and text, show the "Raw" buttons on the file's ObjectBox

Test Plan: View an image and a text file in Diffusion, click on the download link in each.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4467

Differential Revision: https://secure.phabricator.com/D8302
2014-02-21 15:21:38 -08:00
epriestley
d94c33b61a Use a more modern design for "no reviewers" warning in Differential
Summary: Ref T2222. Currently this is a giant header box thing. Move it into the ObjectBox.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8301
2014-02-21 15:10:58 -08:00
epriestley
b62420e6e4 Separate reading object values out of didSetObject() in CustomField
Summary:
Ref T3886. Broadly, fields break down into two types right now: fields which store data on the object (like `DifferentialTitleField`) and fields which store data in custom field storage.

The former type generally reads data from the object into local storage prior to editing, then writes it back afterward. Currently, this happens in `didSetObject()`.

However, now that we load and set objects from ApplicationTransactionQuery, we'll do this extra read-field-values on view interfaces too. There, it's unnecessary and sometimes throws data-attached exceptions.

Instead, separate these concepts, and do all the read-from-object / read-from-storage in one logical chunk, separate from `didSetObject()`.

Test Plan:
  - Edited Differential revision.
  - Edited Maniphest task.
  - Edited Project.
  - Edited user profile.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8299
2014-02-21 14:44:01 -08:00
Chad Little
ce5eafe7f1 Move "Open in Editor" to File Box
Summary: Moves this single action to the File Contents box in Diffusion Browse. Also fixes a PHUIObjectBox missing when enable highlighting is on.

Test Plan: Enable/Disable Highlighting. See disabled Editor button.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4467

Differential Revision: https://secure.phabricator.com/D8300
2014-02-21 14:43:24 -08:00
Bob Trahan
2a277bbc03 Hide upload JS stuff if user isn't logged in
Summary: ...cuz it won't work. I think adding a "login to upload" has no real value as this is a pretty unexpected / power user feature anyway. Fixes T4354.

Test Plan: tried to upload as a logged out user to Phabricator home. my browser just loaded the file as expected.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4354

Differential Revision: https://secure.phabricator.com/D8298
2014-02-21 13:04:23 -08:00
epriestley
a84cc1ab73 Fix (?) an issue with CCs not being written into commit messages properly
Summary: Report from @zeeg, I think this is the root issue. Currently, if a project is CC'd we'll write "CC: projectname", but should write "CC: #projectname".

Test Plan: Verified that we now write "CC: #projectname".

Reviewers: btrahan, zeeg

Reviewed By: zeeg

CC: zeeg, aran

Differential Revision: https://secure.phabricator.com/D8296
2014-02-21 12:56:38 -08:00
Bob Trahan
20a3ee24f9 Herald - add application search for transcripts
Summary: this diff also makes the "test console" appear with the main search nav *and* updates application search to use the page title as the crumb rather than just search. Fixes T4399.

Test Plan: queried for transcript ids - success! queried for TX and MX - success! saved the TX and MX query and it worked again!

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4399

Differential Revision: https://secure.phabricator.com/D8297
2014-02-21 12:51:25 -08:00
epriestley
a24605432f Use standard rendering and controller for Differential subscriptions
Summary:
Ref T2222. Differential has custom code for managing subscriptions, but no longer requires it.

The one trick is that we don't have a hook for loading related data on the subscriptions workflow right now. Just glue that in for the moment; it's relatively harmless, and once Diffusion converts we'll have more context on how to best surface it properly.

Test Plan: Subscribed and unsubscribed from a revision. Viewed different revisions and saw correct subscription state.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8293
2014-02-21 11:55:35 -08:00
epriestley
c5ba75ee9e Implement a "Reviewers" CustomField
Summary:
Ref T3886:

  - Adds a "Reviewers" field as a modern CustomField.

Ref T418:

  - Allows CustomFields to emit transaction metadata.

Test Plan: {F116254}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418, T3886

Differential Revision: https://secure.phabricator.com/D8291
2014-02-21 11:54:32 -08:00
epriestley
aa7ba4c6e6 Implement Differential subscribers as a CustomField
Summary: Ref T3886. Now that a custom field can emit a core transaction, just emit a subscribers transaction.

Test Plan: {F116014}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8289
2014-02-21 11:54:08 -08:00
epriestley
f91e94eb90 Implement view and edit policies in Differential CustomFields
Summary:
Ref T3886. Ref T418.

  - Adds "View Policy" and "Edit Policy" fields.
  - Allows CustomFields to produce arbitrary types of transactions, so these fields can produce standard view/edit policy transactions and get all the strings and validation associated with them.

Test Plan: {F116001}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418, T3886

Differential Revision: https://secure.phabricator.com/D8287
2014-02-21 11:53:48 -08:00
epriestley
01572d9d93 Implement "Repository" as a new-style CustomField in Differential
Summary:
Ref T3886. Ref T418.

  - Adds new capabilities for CustomField:
    - Controls can now bulk-load PHIDs (e.g., for tokenizers).
    - Transactions can now bulk-load PHIDs (e.g., for relationship changes).
  - Implements "Repository" control.
  - Improves tokenizer StandardCustomField controls.

Test Plan:
{F115942}

{F115943}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418, T3886

Differential Revision: https://secure.phabricator.com/D8286
2014-02-21 11:53:37 -08:00
epriestley
05f2ab32d8 Implement "Test Plan" as a CustomField
Summary: Ref T3886. Moves some of the "required" logic to the base class ("DifferentialCoreField") so Title and Test Plan can share it.

Test Plan: Edited revisions using "pro" editor, saw test plan transactions.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8285
2014-02-21 11:53:27 -08:00
epriestley
88227d26bc Allow CustomField to provide ApplicationTransaction change details
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.

Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.

Test Plan:
{F115918}

  - Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T418

Differential Revision: https://secure.phabricator.com/D8284
2014-02-21 11:53:04 -08:00
epriestley
be262f4b0c Implement new-style "Summary" field
Summary:
Ref T3886.

  - Adds "Summary" field.
  - Adds "CoreField" for fields stored on the actual object, to reduce code duplication a bit for the main fields.

Test Plan: {F115902}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8283
2014-02-21 11:52:52 -08:00
epriestley
92ac8b5ad9 Make Differential inline comment rendering more consistent and somewhat more modern
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.

Test Plan: See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: zeeg, aran

Maniphest Tasks: T1790, T2222

Differential Revision: https://secure.phabricator.com/D8294
2014-02-21 11:52:03 -08:00
epriestley
9384c9e36b Don't consider integer CustomFields as changed when the number doesn't change
Summary: Fixes T4466. We do an excessively strict effect check now, which means that these fields changing from (string) "16" to (int) 16 generates a transaction. Instead, compare integer values if the field has data in it.

Test Plan:
{F116261}

(Also made updates without changing the number, which did not appear in the transaction log anymore.)

Reviewers: btrahan, richardvanvelzen

Reviewed By: richardvanvelzen

CC: aran

Maniphest Tasks: T4466

Differential Revision: https://secure.phabricator.com/D8292
2014-02-21 08:59:31 -08:00
epriestley
55a94d8aba Don't prompt to upgrade unset passwords
Summary:
Fixes T4463. When your VCS or account password is not set, we test it for upgrade anyway. This doesn't make sense and throws shortly into the process because the empty hash isn't parseable.

Instead, only show upgrade prompts when the password exists.

Test Plan:
  - Added a password to an existing account with no password via password reset.
  - Added a VCS password to an existing account with no VCS password.
  - Observed no fatals / nonsense behaviors.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4463

Differential Revision: https://secure.phabricator.com/D8282
2014-02-20 08:12:04 -08:00
Chad Little
65a3aa0cc7 Remove phui-box-inner
Summary: When we removed the shadow, we no longer needed two containers.

Test Plan: Browsed Box example, a diff, a task, and other random pages. Grep for phui-box-inner, not used elsewhere.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8281
2014-02-19 23:05:42 -08:00
Bob Trahan
c904e04f89 Fix attachObject bug
Summary: so it was said in IRC and so it is true

Test Plan: saving maniphest tasks with custom fields no longer barfs

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8279
2014-02-19 11:56:41 -08:00
John Watson
1845798771 Fix call to getRefType on non-object in PhabricatorRepositoryEngine.php
Summary:
Error:
  Fatal error: Call to a member function getRefType() on a non-object in /opt/phabricator/phabricator/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php on line 197

Test Plan: No more error in daemon.log afterwards

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8278
2014-02-19 11:55:46 -08:00
epriestley
50ed42761c Don't issue a bazillion queries to load Differential object lists
Summary:
Ref T3496. Currently, we call loadAssets() on each revision table, which invokes a new revision query and a pile of subqueries.

Instead, add `needFlags()` and `needDrafts()` to `RevisionQuery`. Some day these could perhaps be more generic.

Test Plan:
  - Viewed home, differential, etc., no longer saw 9203809238 queries being run for no reason.
  - Drafts and flags still appear properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3496

Differential Revision: https://secure.phabricator.com/D8277
2014-02-18 17:57:45 -08:00
epriestley
65bc2b1ac5 Implement "Pro" version of revision editor, with one field
Summary:
Ref T3886. I spent a few hours trying to make `DifferentialFieldSpecification` extend `PhabricatorCustomField` so I could be more blunt in my approach here and just swap the whole thing over in one go (more or less like I did with Maniphest) but we have a ton of custom fields and things felt really shaky and the change was enormous and hard to keep track of.

Instead, I'm going to do this more gradually and go field-by-field. This implements a CustomField version of the "Title" field.

(There are no links to this in the UI.)

Test Plan:
{F115353}

{F115354}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8276
2014-02-18 16:32:55 -08:00
epriestley
79fa0b0397 Make projects appear in all mailable tokenizers
Summary: Ref T4361. Projects are mailable now, so let them show up in mail contexts.

Test Plan: Added a project as a CC to a task, filtered by project CCs, etc.

Reviewers: btrahan, zeeg, dctrwatson

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4361

Differential Revision: https://secure.phabricator.com/D8274
2014-02-18 16:32:41 -08:00
epriestley
3103ce33b8 Load and attach objects when loading application transactions
Summary:
Ref T3886. Fixes the removed TODO. This also implements the generally reasonable policy "you have to be able to see an object in order to see its transactions". That was implicit before (we never load transactions without loading an object first) but is now explicit.

This fixes bad (nonspecialized) rendering of custom field transactions in Projects, and shortly in Differential, where stories would read "alincoln edited this object." instead of a more specific string.

Test Plan: Viewed a project edit, saw a more specific string. Browed ApplicationTransaction applications.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8273
2014-02-18 16:32:35 -08:00
Bob Trahan
dcd7a316d2 Differential - add DifferentialDraft to track whether revisions have draft feedback or not
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.

Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3496

Differential Revision: https://secure.phabricator.com/D8275
2014-02-18 16:25:16 -08:00
epriestley
b96ab5aadf Modernize VCS password storage to use shared hash infrastructure
Summary: Fixes T4443. Plug VCS passwords into the shared key stretching. They don't use any real stretching now (I anticipated doing something like T4443 eventually) so we can just migrate them into stretching all at once.

Test Plan:
  - Viewed VCS settings.
  - Used VCS password after migration.
  - Set VCS password.
  - Upgraded VCS password by using it.
  - Used VCS password some more.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8272
2014-02-18 14:09:36 -08:00
epriestley
5c84aac908 Allow hashers to side-grade hashes across cost settings
Summary:
Ref T4443. In addition to performing upgrades from, e.g., md5 -> bcrypt, also allow sidegrades from, e.g., bcrypt(cost=11) to bcrypt(cost=12). This allows us to, for example, bump the cost function every 18 months and stay on par with Moore's law, on average.

I'm also allowing "upgrades" which technically reduce cost, but this seems like the right thing to do (i.e., generally migrate password storage so it's all uniform, on average).

Test Plan:
  - Fiddled the bcrypt cost function and saw appropriate upgrade UI, and upgraded passwords upon password change.
  - Passwords still worked.
  - Around cost=13 or 14 things start getting noticibly slow, so bcrypt does actually work. Such wow.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8271
2014-02-18 14:09:36 -08:00
epriestley
580bcd0d2b Implement bcrypt hasher, transparent login upgrade, and explicit upgrade for passwords
Summary:
Ref T4443.

  - Add a `password_hash()`-based bcrypt hasher if `password_hash()` is available.
  - When a user logs in using a password, upgrade their password to the strongest available hash format.
  - On the password settings page:
    - Warn the user if their password uses any algorithm other than the strongest one.
    - Show the algorithm the password uses.
    - Show the best available algorithm.

Test Plan: As an md5 user, viewed password settings page and saw a warning. Logged out. Logged in, got upgraded, no more warning. Changed password, verified database rehash. Logged out, logged in.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8270
2014-02-18 14:09:36 -08:00
epriestley
5778627e41 Provide more storage space for password hashes and migrate existing hashes to "md5:"
Summary: Ref T4443. Provide more space; remove the hack-glue.

Test Plan: Logged out, logged in, inspected database.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8269
2014-02-18 14:09:36 -08:00
epriestley
3c9153079f Make password hashing modular
Summary:
Ref T4443. Make hashing algorithms pluggable and extensible so we can deal with the attendant complexities more easily.

This moves "Iterated MD5" to a modular implementation, and adds a tiny bit of hack-glue so we don't need to migrate the DB in this patch. I'll migrate in the next patch, then add bcrypt.

Test Plan:
  - Verified that the same stuff gets stored in the DB (i.e., no functional changes):
    - Logged into an old password account.
    - Changed password.
    - Registered a new account.
    - Changed password.
    - Switched back to master.
    - Logged in / out, changed password.
    - Switched back, logged in.
  - Ran unit tests (they aren't super extensive, but cover some of the basics).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, kofalt

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8268
2014-02-18 14:09:36 -08:00
Chad Little
2eeef339bf Add crumbs to calendar
Summary: Crumbs everywhere. Not sure how to better format the date, let me know.

Test Plan: Browse everything I can see in Calendar

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4440

Differential Revision: https://secure.phabricator.com/D8265
2014-02-17 16:08:50 -08:00
Chad Little
e537c76106 Add Event Description to Event Page
Summary: Let's people know what the event is, specifically.

Test Plan: View an Event Page

Reviewers: epriestley, btrahan

Reviewed By: btrahan

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8267
2014-02-17 16:08:25 -08:00
epriestley
18f856ac6f Add a "Send Test Notification" button to make testing the server easier
Summary: Ref T4324. Currently, it's a bit of a pain to send yourself notifications, and often involves multiple browsers. Instead, add a button to send them.

Test Plan: {F114495}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4324

Differential Revision: https://secure.phabricator.com/D8255
2014-02-17 16:00:33 -08:00
epriestley
6740082df9 Slightly modernize Aphlict server status page
Summary:
Ref T4324. Add a real `Application` class. Use modern UI elements.

@chad, we could use an icon :3

Test Plan: {F114477}

Reviewers: chad, btrahan

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T4324

Differential Revision: https://secure.phabricator.com/D8254
2014-02-17 16:00:19 -08:00
epriestley
260eb5344b Allow Aphlict to load Javelin and use Javelin class definitions
Summary:
Ref T4324. The server code is probably going to get a fair amount more complicated, so allow it to load Javelin classes in a mostly-reasonable way.

This integration has a few warts, but should be good enough to let us manage complexity through the next iteration of the server.

(Mostly I just want the concicse Javelin mechanism for defining new classes.)

Version bump is just so I can figure stuff out if this creates any issues for users based on which version of things they're running.

Test Plan: Started server, posted some messages through it.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4324

Differential Revision: https://secure.phabricator.com/D8253
2014-02-17 16:00:01 -08:00
epriestley
1b8e129145 Move message posting to PhabricatorNotificationClient
Summary: Ref T4324. Centralize communication with the notification server. This will probably get less messy eventually.

Test Plan: Posted some messages.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4324

Differential Revision: https://secure.phabricator.com/D8252
2014-02-17 15:59:51 -08:00
epriestley
a1e7a4ccca Version the Aphlict notification server and prompt users to upgrade if they're out of date
Summary: Ref T4324. Add some version information to the server status output, and setup checks to test for an unreachable or out-of-date server.

Test Plan:
  - With server down, hit reasonable setup check.
  - With server up and at a bad version, hit reasonable setup check.
  - Viewed `/notification/status/`.
  - The CSS thing fixes this:

{F114445}

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T4324

Differential Revision: https://secure.phabricator.com/D8251
2014-02-17 15:59:39 -08:00
epriestley
f3cbc0e006 Move many task status hardcodes into ManiphestTaskStatus
Summary:
Ref T1812. This cleans up most of the easy hard-coded references to specific statuses:

  - The "fixes" language moves into ManiphestTaskStatus.
  - Add a method to list open statuses.
  - Add a method to test if a status is open.
  - Add a method to get default status for new tasks.

Test Plan: Browsed around, lint, grep, created, filtered and updated tasks.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8264
2014-02-17 15:59:31 -08:00
epriestley
8a725ece0f Bring "fixes x as y" parser forward and use new parsers instead of old ones
Summary: Fixes T3872. Ref T1812. Ref T3886. Modernize the "closes x as y" string parser, and use all the new parsers instead of the old ones.

Test Plan: Made a commit full of a pile of these trigger strings, then used `scripts/repository/reparse.php --message` to reparse it. Verified that parses came back as expected using a bunch of `var_dump()`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1812, T3872, T3886

Differential Revision: https://secure.phabricator.com/D8263
2014-02-17 15:59:13 -08:00
epriestley
5afddb6703 Bring "Reverts X" to more general infrastructure and port unit tests
Summary:
Ref T3886. See D8261. This brings the "reverts x" phrase to modern infrastructure. It isn't actually called by the real parser yet, I'm going to do that in one go at the end so I can test everything more easily.

This had unit tests; port most of them forward.

Test Plan: Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8262
2014-02-17 15:58:59 -08:00
Bob Trahan
e4d60bbc15 Conpherence - add draft support
Summary: Fixes T3497.

Test Plan: on conpherence 1, typed some stuff. clicked conpherence 2 - observed some stuff gone. clicked conpherence 1 - stuff came back! submitted conpherence 1 and reloaded - stuff did not come back.  (Generally played around a bunch like this)

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3497

Differential Revision: https://secure.phabricator.com/D8266
2014-02-17 15:57:13 -08:00
epriestley
d016cac915 Modularize parser for "Closes task X as Y"
Summary:
Ref T3886. Ref T3872. Ref T1812. We have several parsers which look for textual references to other objects, like:

  Closes Tx.
  Depends on Dy.
  Reverts Dz.

Currently, these are pretty hard coded, don't get all the edge cases right, and don't generalize well. They're also implemented in the middle of Differential's field code. So I want to:

  - Share more code so that, e.g., "Tx, Ty" always works (only some rules support it right now);
  - fix bugs in the parser, like T3872;
  - make this a modular, extensible process which runs against custom fields, not a builtin part of fields;
  - make the internals more flexible to accommodate custom stuff like T1812.

This implements the "Verbs optional-noun Object, Optional Other Objects optional-as-something." grammar in a general way so subclasses can just plug in their keywords. Runtime code doesn't touch this yet.

Test Plan: Ran unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3872, T1812, T3886

Differential Revision: https://secure.phabricator.com/D8261
2014-02-17 15:53:47 -08:00
epriestley
7ff539b729 Fix a bad call in ElasticSearch recovery code
Summary: Ref T4446. Some discussion in IRC. Prior to hitting the 1.0.0 issue, we hit and resolved this issue; this is a leftover call from bringing ApplicationSearch to main search.

Test Plan: User confirmed this fixes the issue.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4446

Differential Revision: https://secure.phabricator.com/D8260
2014-02-17 11:39:58 -08:00
epriestley
51acc22962 Merge some of the project editing options into an edit UI
Summary: Ref T4426. This moves "Edit Details", "Edit Picture", and "Archive" to a separate "Edit" interface. "History" becomes part of this UI.

Test Plan:
{F114417}

{F114418}

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4426

Differential Revision: https://secure.phabricator.com/D8248
2014-02-16 20:17:52 -08:00
Chris Colborne
ac09a1a245 Fix Create New Repository link on Repositories
Summary: See <https://github.com/facebook/phabricator/pull/510>.

After the repository rework, the Create New Repository link in Repositories goes straight to creating a phabricator hosted repo(diffusion/create), rather than the chooser create/import (diffusion/new)

I updated it to point to diffusion/new the same as the New Repository link in Diffusion.

As a side note, the Repositories page could probably use the Crumbs treatment.

Reviewed by: epriestley
2014-02-16 20:12:09 -08:00
Chad Little
1c3373d77d Add owner to task boards, tidy UI
Summary: Displays task owner, hides grip texture.

Test Plan: Visit a board in my sandbox, grab and move things.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8246
2014-02-16 09:26:22 -08:00
Chad Little
32cff58cd2 Modernize Calendar View
Summary: Updates Calendar View to more modern components.

Test Plan: Browse Calendar Forward and Back, Create a Status, Get Excited, Get PUMPED.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4392

Differential Revision: https://secure.phabricator.com/D8247
2014-02-16 09:25:29 -08:00
James Rhodes
569a5be561 Fix issue where accessing HTTP domain would override HTTPS cookie
Summary: This fixes an issue where visiting http://code.redpointsoftware.com.au/ would log you out of https://code.redpointsoftware.com.au/

Test Plan: Applied this patch to a live server and saw the issue go away.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8244
2014-02-15 12:20:46 +11:00
James Rhodes
34fae7e3d1 Fix for Phragment ZIP controller
Summary: This applies a minor fix for the Phragment ZIP controller where it would raise missing index errors when attempting to map deleted fragments to file PHIDs (since deleted fragments no longer have files).

Test Plan: Tested this patch on a live server and saw the issue go away.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8243
2014-02-15 11:48:07 +11:00
epriestley
04ba976a04 Remove references to legacy comment IDs
Summary:
Ref T2222. I want to stage a "later" patch to drop this column, but get rid of the last few references to it.

One of these methods has no callers, and the other stuff I've updated to use the modern fields.

Test Plan: Created some inlines, hit "edit", submitted them, `grep.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8240
2014-02-14 15:56:25 -08:00
epriestley
e18b161464 Don't show closed typeahead results while open results exist
Summary:
Ref T4420. When a result list contains both open and closed results, hide the closed results. I think this has a good chance of almost always working, and feeling very intuitive. It has a small chance of being a weird mess. It feels reasonable to me so far

The one bad case I can come up with here is that if you have results which shadow each other, like "Apples" (a closed project) and "Apples and Bananas" (an open project), it is impossible to get "Apples" in the result list, because "Apples and Bananas" will always shadow it. Let's wait for someone to hit this before we figure out how to deal with it.

Test Plan: Typed through open stuff to hit closed stuff.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T4420

Differential Revision: https://secure.phabricator.com/D8238
2014-02-14 15:16:01 -08:00
epriestley
7a28ca847d When there are no events in a timleine, render a single spacer
Summary: Ref T2222. I accidentally changed the beahvior of this in D8229.

Test Plan: Revisions with no comments no longer with too little space between the main object box and the "Revision Update History".

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8239
2014-02-14 14:43:17 -08:00
epriestley
463552ade3 Respect project closure states in fulltext search and handles
Summary: See thread; these are just bugs. Handles and main search do not mark projects correctly as open/closed.

Test Plan: Searched for projects and observed they respect the open/closed flags properly after reidnexing.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8237
2014-02-14 13:52:48 -08:00
epriestley
23146f2bb9 Render Differential previews through ApplicationTransactions
Summary: Ref T2222. Use new code for rendering. Delete `DifferentialRevisionCommentView`, which has no remaining callsites.

Test Plan: Went through all the different actions and verified the previews rendered correctly. Reloaded page to test draft behavior.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8236
2014-02-14 13:16:58 -08:00
John Watson
dbe3818d8c Add ability to pass width arg to dot remarkup interpeter
Summary:
Allows to keep really wide graphs inside the div:

  dot (width=100%) {{{ }}}

Test Plan:
Created a graph that is wider than a phriction page.
Added `(width=100%)` and now the images stays within the div.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8235
2014-02-14 13:15:19 -08:00
epriestley
a0262c0b4f Remove tokenizer.ondemand, and always load on demand
Summary:
Ref T4420. Tokenizers currently operate in "preload" or "ondemand" modes. In the former mode, which is default, they'll try to load the entire result list when a page loads.

The theory here was that this would slightly improve the experience for small installs, and once they got big enough they could switch to "ondemand". In practice, several issues have arisen:

  - We generally don't have a good mechanism for telling installs that they should tweak perf config -- `metamta.send-immediately` is the canonical example here. Some large installs are probably affected negatively by not knowing to change this setting, and having settings like this is generally annoying.
  - We have way way too much config now.
  - With the advent of ApplicationSearch, pages like Maniphest make many redundant loads to prefill sources like projects. Most of the time, this data is not used. It's far simpler to switch everything to ondemand than try to deal with this, and dealing with this would mean creating two very complex divergent pathways in the codebase for a mostly theoretical performance benefit which only impacts tiny installs.
  - We've been using `tokenizer.ondemand` forever on `secure.phabricator.com` since we have many thousands of user accounts, and it doesn't seem sluggish and works properly.

Removing this config is an easy fix which makes the codebase simpler.

I've retained the ability to use preloaded sources, since they may make sense in some cases (in at least one case -- task priorities -- adding a static source pathway might make sense), and they're part of Javelin itself. However, the code will no longer ever go down that pathway.

Test Plan: Used `secure.phabricator.com` for years with this setting enabled.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4420

Differential Revision: https://secure.phabricator.com/D8232
2014-02-14 10:24:40 -08:00
epriestley
87012d60f1 Show icons and disabled/archived/closed results in typahead dynamic list
Summary:
Ref T4420. This is mostly a design change, but addresses two functional issues:

  # Many sources exclude disabled accounts, system agents, archived projects, etc. It is rare to select these, but excluding them completely is too severe, and we've made more than a handful of changes over time to replace a "users" endpoint with an "accounts" endpoint (to include disabled users) or similar. Instead, always show these results, but sort them last and use a special style to clearly mark them as closed, disabled, or otherwise unusual.
    - As a practical consequence, all the similar endpoints can now be merged, so "accounts" and "users" return the exact same result sets.
  # Increasingly, sources can return multiple object types in a single list. For example, "CC" can have a user or mailing list, and soon a project or repository. However, the result list is fairly homogenous across types and it isn't easy to quickly pick out projects vs users. To help with this, add icons showing the result type.

Test Plan:
{F113079}

(The main search results get touched here too, I verified they didn't blow up.)

Reviewers: chad, btrahan

Reviewed By: chad

CC: chad, aran, mbishopim3

Maniphest Tasks: T4420

Differential Revision: https://secure.phabricator.com/D8231
2014-02-14 10:24:11 -08:00
epriestley
3c19f363bf Provide result type information in tokenizers
Summary:
Ref T1279. The new dual-mode user/project tokenizers are a bit disorienting. Provide content type hints.

Very open to any suggestions here, most of this patch is just getting the right data in the right places. We can change things up pretty easily.

  - I like the little icons in the tokens themselves, I think they look good and are useful.
  - I'm less sold on the '(Project)' thing I did in the dropdown. We can easily make this richer if you have thoughts on it -- we could put icons in the left column maybe? Or right-justify the types?
  - I made it always sort users above projects.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: chad, aran, carl

Maniphest Tasks: T4420, T1279

Differential Revision: https://secure.phabricator.com/D7250
2014-02-14 10:23:56 -08:00
epriestley
75c4a185a9 Begin modularizing typeahead sources
Summary:
Ref T4420. This sets up the basics for modular typeahead sources. Basically, the huge `switch()` is just replaced with class-based runtime dispatch.

The only clever bit I'm doing here is with `CompositeDatasource`, which pretty much just combines the results from several other datasources. We can use this to implement some of the weird cases where we need multiple types of results, although I think I can entirely eliminate many of them entirely. It also makes top-level implementation simpler, since more logic can go inside the sources.

Sources are also application-aware, will be responsible for placeholder text, and have a slightly nicer debug view.

Test Plan: {F112859}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4420

Differential Revision: https://secure.phabricator.com/D8228
2014-02-14 10:23:25 -08:00
epriestley
7a96de10f0 Hide seen transactions by default in all modern applications
Summary:
Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions.

This might need some #design help.

Test Plan:
The tricky part here is the anchor rule, which deals with the case where you follow a link to `T123#4`, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest:

{F112891}

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8229
2014-02-14 10:23:07 -08:00
Chad Little
390a0f91b0 Remove spacing after timeline on diffs
Summary: I assume this box is always after timeline

Test Plan: test a diff or two

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8230
2014-02-13 21:02:19 -08:00
epriestley
4e8e035de0 Show detailed ApplicationTransaction changes in a dialog
Summary: Fixes T4423. This works better and is simpler and more flexible.

Test Plan: Clicked "(Show Details)", got a popup with the details.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T4423

Differential Revision: https://secure.phabricator.com/D8227
2014-02-13 19:37:31 -08:00
epriestley
afa227ca98 Link the "(board)" text after project references in Maniphest
Summary: Fixes T4429. Shows and allows you to click through on board links when a task appears on workboards.

Test Plan:
{F112837}

Clicked the links.

Reviewers: chad, btrahan

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T4429

Differential Revision: https://secure.phabricator.com/D8226
2014-02-13 18:12:38 -08:00
epriestley
005d194cd2 Automatically inline small images using data URIs
Summary: Fixes T3300. Images under 32KB are inlined automatically into CSS using data URIs; larger images remain as normal links. I picked the 32KB threshold arbitrarily, based on it looking roughly like it got reasonable results on `core.pkg.css` (inlining most of the random stuff, but not inlining all the 2X sprites and such).

Test Plan: Loaded site, browsed around, looked at generated CSS.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3300

Differential Revision: https://secure.phabricator.com/D8225
2014-02-13 17:39:47 -08:00
epriestley
e88d0f9c42 Merge Differential comment migration to ApplicationTransactions
Summary: Ref T2222. This merges the `tmp.differential` branch, including the
comment -> application transaction migration, to `master`.

Auditors: btrahan
2014-02-13 17:04:53 -08:00
epriestley
7374a0a4d4 Fix inline comment links for non-visible comments
Summary: Ref T2222. Restore this funky is-visible / inline-is-elsewhere logic.

Test Plan: Updated a revision, saw all the inlines render properly when looking at various diffs and versus-diffs. Clicked inline links.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8224
2014-02-13 16:52:48 -08:00
epriestley
9afe52de51 Provide transaction strings, icons and colors for Differential
Summary: Ref T2222. Once these are live, yell if any of them seem off. I tried to mostly stay consistent-ish with what we had before.

Test Plan: Looked at a bunch of revisions and saw more detailed, colorful transactions.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8223
2014-02-13 15:51:26 -08:00
epriestley
cb20205aee Don't show "edit" links in Differential for now
Summary: Ref T2222. These don't work yet. We just have to copy a couple fields, but let's sort that out later since this is purely a new feature.

Test Plan: Looked at a revision, no edit links.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8222
2014-02-13 15:51:26 -08:00
epriestley
c7ca3cd4ab Pass $all_changesets, not $changesets, to inline rendering logic
Summary: `$all_changesets` has all the changesets.

Auditors: btrahan
2014-02-13 15:12:42 -08:00
epriestley
ee38e33496 Fix two issues with Differential Transactions
Summary: With symbols, the transactions fataled on `getID()`. Also my line
number sorting was a bit goofy.

Auditors: btrahan
2014-02-13 15:06:38 -08:00
epriestley
0159315763 Add some missing text for Differential inline transactions
Summary: Ref T2222. Add inline and default text.

Test Plan: Feed works again.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8218
2014-02-13 15:00:29 -08:00
epriestley
348b07a4be Make Asana bridge work with new Differential transactions
Summary: Ref T2222. Unbreak the Asana/JIRA bridge.

Test Plan: {F112712}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8217
2014-02-13 15:00:29 -08:00
epriestley
29918e1bd4 Fix double-rendering of inline comments in mail
Summary: Ref T2222. This is a `tmp.differential`-only issue. Inline comment transactions now have content, so we treat them like body text. We also render them separately as inline text. This produces mail where inlines are rendered twice.

Test Plan: Sent myself mail, saw only one copy of inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8216
2014-02-13 15:00:29 -08:00
epriestley
62cb584083 Use timeline view in Differential and make inlines somewhat usable again
Summary:
Ref T2222. This gets rid of Differential's custom view and uses a standard view instead.

This also mostly fixes the rendering logic for inlines.

This is headed to the `tmp.differential` branch.

Test Plan: {F112696}

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T1790, T2222

Differential Revision: https://secure.phabricator.com/D8215
2014-02-13 15:00:29 -08:00
Chad Little
a4529b4e60 ProjectView improvements
Summary: Simplified header, added Workboard button and icon, moved Maniphest actions to "Open Tasks" Object Box. Reduced actions by 3.

Test Plan: Test a number of project pages, looks better, cleaner.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8219
2014-02-13 14:36:49 -08:00
epriestley
30ffe0ce5d Bump number of visible Maniphest tags to 4 (from 2)
Summary: Ref T3574. Since this list just clips in a totally reasonable way on mobile and we got another user request for it, let's bump this to 4 for now and we can refine mobile later.

Test Plan: Looked at list on desktop; saw 4 tags before truncation. Looked at list on mobile, saw reasonable clipping behavior which didn't mar usability.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T3574

Differential Revision: https://secure.phabricator.com/D8213
2014-02-13 10:05:47 -08:00
epriestley
0dc73d5d93 Always provide a valid content source for DifferentialCommentEditor
Summary: Ref T2222. On the `tmp.differential` branch, we're currently having
issues parsing commits which reference Differential revisions, because the
"user closed this revision (closed by commit xyz)" message is fataling:

  [2014-02-13 14:12:36] EXCEPTION: (PhutilProxyException) Error while
  executing task ID 345358 from queue. {>} (AphrontQueryException)
  #1048: Column 'contentSource' cannot be null

Specifically, the MessageParser pathway for CommentEditor doesn't set a content
source. Make sure CommentEditor always sets a content source.

(This is also causing a buildup of diffs on D8212 and D8211.)

Auditors: btrahan
2014-02-13 06:19:55 -08:00
epriestley
18938b5310 Migrate Differential comments to ApplicationTransactions
Summary:
Ref T2222. This is the big one.

This migrates each `DifferentialComment` to one or more ApplicationTransactions (action, cc, reviewers, update, comment, inlines), and makes `DifferentialComment` a double-reader for ApplicationTransactions.

The migration is pretty straightforward:

  - If a comment took an action not otherwise covered, it gets an "action" transaction. This is something like "epriestley abandoned this revision.".
  - If a comment updated the diff, it gets an "updated diff" transaction. Very old transactions of this type may not have a diff ID (probably only at Facebook).
  - If a comment added or removed reviewers, it gets a "changed reviewers" transaction.
  - If a comment added CCs, it gets a "subscribers" transaction.
  - If a comment added comment text, it gets a "comment" transaction.
  - For each inline attached to a comment, we generate an "inline" transaction.

Most comments generate a small number of transactions, but a few generate a significant number.

At HEAD, the code is basically already doing this, so comments in the last day or two already obey these rules, roughly, and will all generate only one transaction (except inlines).

Because we've already preallocated PHIDs in the comment text table, we only need to write to the transaction table.

NOTE: This significantly degrades Differential, making inline comments pretty much useless (they each get their own transaction, and don't show line numbers or files). The data is all fine, but the UI is garbage now. This needs to be fixed before we can deploy this to users, but it's easily separable since it's all just display code.

Specifically, they look like this:

{F112270}

Test Plan:
I've migrated locally and put things through their paces, but it's hard to catch sketchy stuff locally because most of my test data is nonsense and bad migrations wouldn't necessarily look out of place.
IMPORTANT: I'm planning to push this to a branch and then shift production over to the branch, and run it for a day or two before bringing it to master.

I generally feel good about this change: it's not that big since we were able to separate a lot of pieces out of it, and it's pretty straightforward. That said, it's still one of the most scary/dangerous changes we've ever made.

Reviewers: btrahan

CC: chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8210
2014-02-12 14:34:48 -08:00
epriestley
3092efdc65 Reduce reliance on getRevisionID() on DifferentialComment
Summary: Ref T2222. A few rendering interfaces rely on fishing the revision ID out of a DifferentialComment, but it will only have the PHID soon. Pass in the revision and use it to determine the ID instead.

Test Plan: Browsed, previewed, examined comments. Clicked anchors.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8209
2014-02-12 14:34:26 -08:00
epriestley
143b89286a Remove unreachable DifferentialRevisionStatsView
Summary:
Ref T2222. I wiped out the Differential-specific stats page a long time ago, but missed this. It turned up recently in `grep`.

Facts will eventually fill this role; this code is unreachable; it probably doesn't work now and definitely won't work in a day or two after ApplicationTransactions.

Test Plan: Used `grep` to look for callsites.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8208
2014-02-12 14:33:55 -08:00
epriestley
54ce4a719e Fix issue where new transaction comments can be spread across multiple transactions
Summary: Ref T2222. We need this `clone` when constructing the new multi-comments in Differential, or we get double-comments internally. This shows up as emails with double comment text.

Test Plan: Sent some "Accept + comment" emails, only one comment in the body.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8206
2014-02-12 09:06:24 -08:00
Chad Little
db66cd830d PHUITimelineView
Summary:
Updates PhabricatorTimeline to PHUITimeline. Uses standard colors and spacing, softens up the actors, and reduces visual spacing of action-only events.

- Also updated some 2x sprite images.

Test Plan: Tested Tasks Paste and Pholio in my sandbox.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8205
2014-02-12 09:02:05 -08:00
epriestley
8f9b7f4196 Move Differential to proper subscriptions
Summary:
Ref T2222. Ref T4415. We're still writing Differential subscription stuff into this weird legacy `differential_relationship` table, which is like an edge table but extremely ancient.

Move it into a proper table.

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

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

Ran the migration, verified data survived.

Reviewers: btrahan

Reviewed By: btrahan

CC: FacebookPOC, aran

Maniphest Tasks: T2222, T4415

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

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

{F111919}

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

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

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, FacebookPOC

Maniphest Tasks: T2222

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

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

{F111909}

Test Plan: Updated a revision.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

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

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

This change should have no impact on any application behaviors.

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

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8199
2014-02-11 15:21:07 -08:00
Bob Trahan
fec5ea8f9d Pholio - robustify submission errors to retain image edits
Summary: we should build all the image stuff on every post and use that posted image data if there's an error. this diff makes that so. Fixes T4380.

Test Plan: made a mock with no title, tried to save it, and was delighted to see my images still there. edited a mock - removing the title and adding images - verified edits showed up after erroneous submission. added a title and submitted and changes were saved.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4380

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

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

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

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

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8196
2014-02-11 11:34:15 -08:00
epriestley
3a01d6dae5 Fix project editing issue with moving wikis
Summary: Fixes T4409. I didn't get this quite right when I updated it to ApplicationTransactions.

Test Plan: Renamed a project, saw wiki move.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4409

Differential Revision: https://secure.phabricator.com/D8198
2014-02-11 11:30:38 -08:00
Bob Trahan
8739ea4dc1 Diffusion - fix browse file bug
Summary: we were calling a member method on a diffusion hash. not sure why.  Fixes T4402

Test Plan: clicked about, no fatals and seemed to move sensical backwards in time

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4402

Differential Revision: https://secure.phabricator.com/D8194
2014-02-11 10:38:27 -08:00
epriestley
b1243e549c Allow unsubscription from projects
Summary:
Fixes T4379. Several changes:

  - Migrate all project members into subscribers.
  - When members are added or removed, subscribe or unsubscribe them.
  - Show sub/unsub in the UI.
  - Determine mailable membership of projects by querying subscribers.

Test Plan:
  - As `duck`, joined a project.
  - Added the project as a reviewer to a revision.
  - Commented on the revision.
  - Observed `duck` receive mail.
  - Unsubscribed as `duck`.
  - Observed no mail.
  - Resubscribed as `duck`.
  - Mail again.
  - Joined/left project, checked sub/unsub status.
  - Ran migration, looked at database.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, asherkin

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8189
2014-02-11 07:45:56 -08:00
epriestley
4e70664ed4 Fix alternate filtering conditions in object selector dialog
Summary:
These didn't get updated either when the main search got rebuilt. Adjust and modernize them. Also this uses "exclude", which I couldn't find any callsites for but just missed, so restore that.

At some point I plan to swap this whole thing to ApplicationSearch and that will let us get rid of a bunch of stuff.

Test Plan: Searched for all filters, got sensible results, verified source object doesn't show up as a result.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, mbishopim3

Differential Revision: https://secure.phabricator.com/D8188
2014-02-11 07:45:33 -08:00
epriestley
094922bcb9 Support "only the first time" in Maniphest
Summary:
Ref T4403. Implements "only the first time" for Maniphest rules, and fixes the trigger itself.

The trigger would never fire and block rules because it was comparing a string (like "first") to an int (like 0).

The "only" vs "every" stuff is contributed and I should have pushed back harder on this toInt / toString stuff. Maybe I'll just get rid of it; it purely causes confusion and problems.

Test Plan: Wrote an "only the first time" rule, ran it twice, it applied once.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4403

Differential Revision: https://secure.phabricator.com/D8193
2014-02-11 07:45:26 -08:00
epriestley
a3acf5fc4b Soft-launch project boards
Summary: Ref T1344. I don't think we'll need to destroy any data moving forward, and would like to get more feedback about what changes users want.

Test Plan: Looked at a project and clicked onto its board.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T1344

Differential Revision: https://secure.phabricator.com/D8191
2014-02-10 16:13:56 -08:00
Chad Little
1ac28a7127 Remove 'setShadow' from PHUIBoxView
Summary: Removes setShadow, uses setBorder instead

Test Plan: Tested multicolumn, feed, search box, many other areas. Things look less 3d.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8192
2014-02-10 16:04:42 -08:00
Bob Trahan
1830868007 Herald - make herald condition of herald rule display better
Summary: ...by the surprising step of changing how this data is stored from id to phid. Also a small fix to not allow "disabled" rules to be used as herald rule conditions, i.e. can't make a rule that depends on a disabled rule.

Test Plan: viewed existing herald rule that had a rule condition and noted nice new display using handle. made a new rule that had a rule condition and verified it worked correctly.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8186
2014-02-10 14:40:09 -08:00
epriestley
fb9b023fba Rebuild custom field indexes when rebuilding standard/fulltext search indexes
Summary:
Ref T4379. Fixes T4359. Currently, `bin/search index` does not rebuild CustomField indexes. This is because they aren't really part of the main search index. However, from a user's point of view this is by far the most logical place to look for index rebuilds, and it's straightforward for us to write into this secondary store.

At some point, it might be nice to let you specify fields as "fulltext" too, although no one has asked for that yet. We could then dump the text of those fields into the fulltext index. Ref T418.

Test Plan: Used `bin/search index --type proj --trace`, etc., and examination of the database to verify that indexes rebuilt. Reindexed users, tasks, projects.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4359, T418, T4379

Differential Revision: https://secure.phabricator.com/D8185
2014-02-10 14:32:45 -08:00
epriestley
d70dd190e4 Make project profile picture edits transactional
Summary: Ref T4379. Make changing pictures use apptransactions.

Test Plan: {F111554}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8184
2014-02-10 14:32:30 -08:00
epriestley
8c84ed61b1 Migrate project profiles onto projects, and remove ProjectProfile object
Summary:
Ref T4379. Long ago, the "Project" vs "ProjectProfile" split was intended to allow a bunch of special fields on projects without burdening the simple use cases, but CustomField handles that far better and far more generally, and doing this makes using ApplicationTransactions a pain to get right, so get rid of it.

The only remaining field is `profileImagePHID`, which we can just move to the main Project object. This is custom enough that I think it's reasonable not to express it as a custom field.

Test Plan: Created a project, set profile, edited project, viewed in typeahead, ran migration, verified database results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8183
2014-02-10 14:32:14 -08:00
epriestley
1520065ec0 Migrate project blurb/description to standard custom field storage
Summary: Ref T4379. Major goal here is to remove `ProjectProfile` so all edits use ApplicationTransactions. This also makes things more flexible, allowing users to disable this field if they don't like it.

Test Plan: Ran migration, verified data survived, edited/created projects, reordered fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8182
2014-02-10 14:31:57 -08:00
epriestley
7c8a875c19 Implement custom fields in Projects
Summary:
Ref T4379. Ref T3794. Fixes T4010. This brings CustomFields to projects.

My primary goal is to get rid of the special casing around project profiles and profile editing, so all edits are ApplicationTransactions. Particularly, I want to make the "blurb/description" field a custom field which goes through builtin infrastructure.

A distant secondary goal is that this is a feature which users like/want because users like/want features.

Test Plan: Added a custom field and examined it in the edit, view, and search interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3794, T4010, T4379

Differential Revision: https://secure.phabricator.com/D8180
2014-02-10 14:31:34 -08:00
epriestley
e4e4810b89 Make the "you can't edit away your edit capability" policy check generic
Summary:
Ref T4379. Currently, you can edit away your edit capability in Projects. Prevent this in a general way.

Since some objects have complex edit policies (like "the owner can always edit"), we can't just check the value itself. We also can't fairly assume that every object has a `setEditPolicy()` method, even though almost all do right now. Instead, provide a way to pretend we've completed the edit and changed the policy.

Test Plan: Unit tests, tried to edit away my edit capability.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8179
2014-02-10 14:31:16 -08:00
epriestley
c3544f8862 Move "Archive Project" to a standard, separate action
Summary:
Ref T4379. Projects currently include their "delete/disable" function as part of edit, which is atypical. Instead, provide it as a first-class action. This is primarily for consistency between applications.

(The action list on projects is getting pretty huge, but we can deal with that separately; I have some ideas.)

Test Plan: Archived/unarchived a project. Edited a project.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8177
2014-02-10 14:30:47 -08:00