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

6382 commits

Author SHA1 Message Date
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