1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-25 06:50:55 +01:00
Commit graph

13484 commits

Author SHA1 Message Date
epriestley
5a8a56f414 Prepare the new AuthPassword infrastructure for storing account passwords
Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.

Before account passwords can move, we need to make two changes:

  - For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
  - Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.

Then add a nice story about password digestion.

Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18900
2018-01-23 10:57:40 -08:00
epriestley
753c4c5ff1 Remove the "PhabricatorRepositoryVCSPassword" class and table
Summary:
Ref T13043. After D18898, this has been migrated to new, more modern storage and no longer has any readers or writers.

One migration from long ago (early 2014) is affected. Since this is ancient and the cost of dropping this is small (see inline), I just dropped it.

I'll note this in the changelog.

Test Plan: Ran migrations, got a clean bill of health from `storage status`. Grepped for removed symbol.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18899
2018-01-23 10:56:37 -08:00
epriestley
dd8f588ac5 Migrate VCS passwords to new shared password infrastructure
Summary:
Ref T13043. Migrate VCS passwords away from their dedicated table to new the new shared infrastructure.

Future changes will migrate account passwords and remove the old table.

Test Plan:
- Ran migrations.
  - Cloned with the same password that was configured before the migrations (worked).
  - Cloned with a different, invalid password (failed).
- Changed password.
  - Cloned with old password (failed).
  - Cloned with new password (worked).
- Deleted password in web UI.
  - Cloned with old password (failed).
- Set password to the same password as it currently is set to (worked, no "unique" collision).
- Set password to account password. !!This (incorrectly) works for now until account passwords migrate, since the uniqueness check can't see them yet.!!
- Set password to a new unique password.
  - Cloned (worked).
  - Revoked the password with `bin/auth revoke`.
  - Verified web UI shows "no password set".
  - Verified that pull no longer works.
  - Verified that I can no longer select the revoked password.
- Verified that accounts do not interact:
  - Tried to set account B to account A's password (worked).
  - Tried to set account B to a password revoked on account A (worked).
- Spot checked the `password` and `passwordtransaction` tables for saniity.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18898
2018-01-23 10:56:13 -08:00
epriestley
bb12f4bab7 Add test coverage to the PasswordEngine upgrade workflow and fix a few bugs
Summary:
Ref T13043. When we verify a password and a better hasher is available, we automatically upgrade the stored hash to the stronger hasher.

Add test coverage for this workflow and fix a few bugs and issues, mostly related to shuffling the old hasher name into the transaction.

This doesn't touch anything user-visible yet.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18897
2018-01-23 10:55:35 -08:00
epriestley
c280c85772 Consolidate password verification/revocation logic in a new PhabricatorAuthPasswordEngine
Summary:
Ref T13043. This provides a new piece of shared infrastructure that VCS passwords and account passwords can use to validate passwords that users enter.

This isn't reachable by anything yet.

The test coverage of the "upgrade" flow (where we rehash a password to use a stronger hasher) isn't great in this diff, I'll expand that in the next change and then start migrating things.

Test Plan: Added a bunch of unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18896
2018-01-23 10:54:49 -08:00
epriestley
42e2cd9af0 Add a "--force" flag to bin/auth revoke
Summary: Ref T13043. I'd like to replace the manual credential revocation in the Phacility export workflow with shared code in `bin/auth revoke`, but we need it to run non-interactively. Add a `--force` flag purely to make our lives easier.

Test Plan: Ran `bin/auth revoke --everywhere ...` with and without `--force`. Got prompted without, got total annihilation with.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18895
2018-01-22 20:12:36 -08:00
epriestley
9c00a43784 Add a more modern object for storing password hashes
Summary:
Ref T13043. Currently:

  - Passwords are stored separately in the "VCS Passwords" and "User" tables and don't share as much code as they could.
  - Because User objects are all over the place in the code, password hashes are all over the place too (i.e., often somewhere in process memory). This is a very low-severity, theoretical sort of issue, but it could make leaving a stray `var_dump()` in the code somewhere a lot more dangerous than it otherwise is. Even if we never do this, third-party developers might. So it "feels nice" to imagine separating this data into a different table that we rarely load.
  - Passwords can not be //revoked//. They can be //deleted//, but users can set the same password again. If you believe or suspect that a password may have been compromised, you might reasonably prefer to revoke it and force the user to select a //different// password.

This change prepares to remedy these issues by adding a new, more modern dedicated password storage table which supports storing multiple password types (account vs VCS), gives passwords real PHIDs and transactions, supports DestructionEngine, supports revocation, and supports `bin/auth revoke`.

It doesn't actually make anything use this new table yet. Future changes will migrate VCS passwords and account passwords to this table.

(This also gives third party applications a reasonable place to store password hashes in a consistent way if they have some need for it.)

Test Plan: Added some basic unit tests to cover general behavior. This is just skeleton code for now and will get more thorough testing when applications move.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18894
2018-01-22 15:35:28 -08:00
epriestley
fa1ecb7f66 Add a bin/auth revoke revoker for SSH keys
Summary: Ref T13043. Adds CLI support for revoking SSH keys. Also retargets UI language from "Deactivate" to "Revoke" to make it more clear that this is a one-way operation. This operation is already correctly implemented as a "Revoke" operation.

Test Plan: Used `bin/auth revoke --type ssh` to revoke keys, verified they became revoked (with proper transactions) in the UI. Revoked keys from the web UI flow.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18893
2018-01-22 15:35:07 -08:00
epriestley
39c3b10a2f Add a bin/auth revoke revoker for sessions
Summary: Ref T13043. Allows CLI revocation of login sessions.

Test Plan: Used `bin/auth revoke --type session` with `--from` and `--everywhere` to revoke sessions. Saw accounts get logged out in web UI.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18892
2018-01-22 12:01:14 -08:00
epriestley
7970cf0585 Add a bin/auth revoke revoker for temporary tokens
Summary: Ref T13043. Allows CLI revocation of temporary ("forgot password", "one-time login") tokens.

Test Plan: Used "Forgot Password?" to generate tokens, used `bin/auth revoke --type temporary` with `--from` and `--everywhere` to revoke them.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18891
2018-01-22 12:00:33 -08:00
epriestley
a9d7b4f0ff Support bulk edit of "points" for Maniphest tasks
Summary: Ref T13025. Fixes T10973. Fairly straightforward. The "points" type is just an alias for "text" today.

Test Plan: Bulk edited points.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T10973

Differential Revision: https://secure.phabricator.com/D18889
2018-01-22 11:59:52 -08:00
epriestley
d9b6513a21 Respect tokenizer limits in the bulk editor
Summary: Ref T13025. This makes limits (for fields like "Assign To") work in the bulk editor, so you can't type "Assign to: x, y, z" anymore.

Test Plan: Hit limit for "Assign to" and a custom project field. No limit for "Add subscribers".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18888
2018-01-22 11:55:55 -08:00
epriestley
fbfcc37531 Respect token limits for "Assign to" and custom datasource fields in Herald
Summary:
See PHI173. Currently, Herald has an "Assign to" action for tasks, and you can specify custom fields with datasource values (like users or projects) that have a limit (like 1 "Owner", or 12 "Jury Members").

Herald doesn't support these limits right now, so you can write `[ Assign to ][ X, Y, Z ]`. This just means "Assign to X", but make it more clear by actually enforcing the limit in the UI.

Test Plan:
  - Created a "projects" custom field with limit 1.
  - Tried to create actions that 'assign to' or 'set custom field to' more than one thing, got helpfully rebuffed by the UI.
  - Created an "add subscribers" action with more than one value.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18887
2018-01-22 11:54:12 -08:00
epriestley
6a62797056 Fix some issues with Diffusion file data limits
Summary:
See <https://discourse.phabricator-community.org/t/files-created-from-repository-contents-slightly-over-one-chunk-in-size-are-truncated-to-exactly-one-chunk-in-size/988/1>. Three issues here:

  - When we finish reading `git cat-file ...` or whatever, we can end up with more than one chunk worth of bytes left in the internal buffer if the read is fast. Use `while` instead of `if` to make sure we write the whole buffer.
  - Limiting output with `setStdoutSizeLimit()` isn't really a reliable way to limit the size if we're also reading from the buffer. It's also pretty indirect and confusing. Instead, just let the `FileUploadSource` explicitly implement a byte limit in a straightforward way.
  - We weren't setting the time limit correctly on the main path.

Overall, this could cause >4MB files to "write" as 4MB files, with the rest of the file left in the UploadSource buffer. Since these files were technically under the limit, they could return as valid. This was intermittent.

Test Plan:
  - Pushed a ~4.2MB file.
  - Reloaded Diffusion a bunch, sometimes saw the `while/if` buffer race and produce a 4MB file with a prompt to download it. (Other times, the buffer worked right and the page just says "this file is too big, sorry").
  - Applied patches.
  - Reloaded Diffusion a bunch, no longer saw bad behavior or truncated files.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18885
2018-01-22 11:52:37 -08:00
epriestley
c637680445 Show related objects on daemon worker task queue detail pages
Summary:
See PHI287. Currently, you can't get very much information about a task in the worker queue from the web UI.

This is largely intentional (e.g., it's bad if we let you inspect the content of a "send an administrator's password reset email" task), and a lot of the data is task-class specific so it would be a lot of work to expose it, but we can add one useful piece of information pretty easily: for tasks with an `objectPHID` that points at a valid object that's visible to the user, tell the user which object the task is associated with.

Test Plan: {F5386562}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18890
2018-01-20 06:52:15 -08:00
epriestley
3b7547e726 Fix an issue where certain configurations could fail to publish revision feed stories
Summary: See PHI292. This is just a generalization of D18851: feed stories have the same issue as mail. Don't hide "requested a review" in either mail or feed.

Test Plan:
  - Enable prototypes.
  - No harbormaster builds.
  - Create a revision.
    - Pre-patch: no feed story.
    - Post-patch: feed story.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18886
2018-01-19 15:39:31 -08:00
epriestley
86a0c7daa2 Fix a fatal in blame when the viewer can't see a revision because of a permission issue
Summary:
Fixes T13040. To reproduce:

  - View a file with blame enabled, where some line has an associated revision (say, `D123`).
  - Edit `D123` so it exists and is a valid revision, but the viewer can't see it.
  - Reload the page.

Instead, only add revisions to the map if we actually managed to load them.

Test Plan: Page no longer fatals.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13040

Differential Revision: https://secure.phabricator.com/D18884
2018-01-19 14:19:06 -08:00
epriestley
3038d564a6 Allow bulk edits to be made silently if you have CLI access
Summary:
Fixes T13042. This hooks up the new "silent" mode from D18882 and makes it actually work.

The UI (where we tell you to go run some command and then reload the page) is pretty clumsy, but should solve some problems for now and can be cleaned up eventually. The actual mechanics (timeline aggregation, Herald interaction,  etc.) are on firmer ground.

Test Plan:
  - Made a normal bulk edit, got mail and feed stories.
  - Made a silent bulk edit, no mail and no feed.
  - Saw "Silent Edit" marker in timeline for silent edits:

{F5386245}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13042

Differential Revision: https://secure.phabricator.com/D18883
2018-01-19 13:24:54 -08:00
epriestley
8b12fa6d6e Prepare TransactionEditor for silent transactions via bulk edit
Summary:
Ref T13042. This adds a "silent" edit mechanism which suppresses feed stories, email, and notifications.

The other behaviors here are:

  - The transactions are marked as "silent" so we can render a hint in the UI in the future to make it clear to users that they aren't missing email.
  - If the editor uses Herald, mail rules are suppressed so they don't fire incorrectly (this mostly affects "the first time this rule matches, send me an email" rules: without this, they'd match "the first time" on the bulk edit, not send email, then never match again since they already matched).
  - If the edit queues additional edits, those are applied silently too.

This doesn't (or, at least, shouldn't) actually change any behavior since you can't apply silent edits yet.

Test Plan:
Somewhat theoretical, since this isn't reachable yet. Should get meaningful testing in an upcoming change.

Did a bit of var_dump() / debug poking to attempt to verify that nothing too crazy is happening.

Viewed and edited objects, no changes in behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13042

Differential Revision: https://secure.phabricator.com/D18882
2018-01-19 13:23:38 -08:00
epriestley
8a5def8e0a Remove legacy transaction editor "getDisableEmail()" method
Summary:
Ref T13042. This is a very, very old policy-violating option from yesteryear which supported build systems publishing updates by adding comments to revisions, without sending email about it.

Harbormaster has served this role for a long time and this is policy-violating in the general case (it allows attackers to act in secret).

Test Plan: Grepped for affected symbols.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13042

Differential Revision: https://secure.phabricator.com/D18881
2018-01-19 13:23:06 -08:00
epriestley
7a43181337 Organize bulk edit actions into nice groups
Summary: Ref T13025. We're getting kind of a lot of actions, so put them in nice groups so they're easier to work with.

Test Plan: {F5386038}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18880
2018-01-19 13:22:25 -08:00
epriestley
ae1b07bcfb Support "<select />" custom fields in bulk editor
Summary: Ref T13025. Fixes T5689. A straightforward change!

Test Plan: Used the bulk editor to modify a custom "select" field like the one in T5689.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T5689

Differential Revision: https://secure.phabricator.com/D18879
2018-01-19 13:18:02 -08:00
epriestley
f8113aecdc Make bulk edit <select /> fields a little more natrual and set options for subtype transactions
Summary:
Ref T13025. This is some minor technical stuff: make the "select" bulk edit type a little more consistent with other types by passing data down instead of having it reach up the stack. This simplifies the implementation of a custom field "select" in a future change.

Also, provide an option list to the "select" edit field for object subtypes. This is only accessible via Conduit so it currently never actually renders anything in the UI, but with the bulk edit stuff we get some initialization order issues if we don't set anything. This will also make any future changes which expose subtypes more broadly more straightforward.

Test Plan:
  - Bulk edited "select" fields, like "Status" and "Priority".
  - No more fatal when trying to `getOptions()` internally on the subtype field.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18878
2018-01-19 13:17:28 -08:00
epriestley
b6737554e1 Support tokenizer custom fields in bulk editor
Summary:
Ref T13025. This allows custom tokenizer fields, like a "Owning Group" field, to be edited with the bulk editor.

See PHI173 for some context.

Test Plan: Edited a custom "Owner" field (a project tokenizer) with the bulk editor.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18877
2018-01-19 13:16:46 -08:00
epriestley
a26cf20dd1 Fix a bug with setting custom PHID list field values via Conduit and prepare for bulk edits
Summary:
Ref T13025. Custom field transactions work somewhat unusually: the values sometimes need to be encoded. We currently do not apply this encoding correctly via Conduit.

For example, setting some custom PHID field to `["PHID-X-Y"]` fails with a bunch of JSON errors.

Add an extra hook callback so that EditTypes can apply processing to transaction values, then apply the correct CustomField processing.

This only affects Conduit. In a future diff, this also allows bulk edit of custom fields to work correctly.

Test Plan: Added a custom field to Maniphest with a list of projects. Used Conduit to bulk edit it (which now works, but did not before). Used the web UI to bulk edit it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18876
2018-01-19 12:51:35 -08:00
epriestley
1e51f1d0dc Reuse more transaction construction code in bulk editor
Summary:
Ref T13025. Currently, the bulk editor takes an HTTP request and emits a list of "raw" transactions (simple dictionaries). This goes into the job queue, and the background job builds a real transaction.

However, the logic to turn an HTTP request into a raw transaction is ending up with some duplication, since we generally already have logic to turn an HTTP request into a full object.

Instead: build real objects first, then serialize them to dictionaries. Send those to the job queue, rebuild them into objects again, and we end up in the same spot with a little less code duplication.

Finally, delete the mostly-copied code.

Test Plan: Used bulk editor to add comments, projects, and rename tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18875
2018-01-19 12:49:17 -08:00
epriestley
91a78db99b Support "Assign To" in Maniphest bulk editor
Summary:
Ref T13025. See PHI173. This supports the "Assign to" field in the new editor.

This is very slightly funky: to unassign tasks, you need to leave the field blank. I have half a diff to fix this, but the way the `none()` token works in the default datasource is odd so it needs a separate datasource. I'm punting on this for now since it works, at least, and isn't //completely// unreasonable.

This also simplifies some EditEngine stuff a little. Notably:

  - I reorganized EditType construction slightly so subclasses can copy/paste a little bit less.
  - EditType had `field` and `editField` properties which had the same values. I canonicalized on `editField` and made this value set a little more automatically.

Test Plan: Used bulk editor to reassign some tasks. By leaving the field blank, unassigned tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18874
2018-01-19 12:48:41 -08:00
epriestley
0cad6021b6 Restore "Tags" and "Subscribers" edit capabilities to Maniphest bulk editor
Summary: Depends on D18867. Ref T13025. Fixes T8740. Rebuilds the tag/subscriber actions (add, remove, set) into the bulk editor.

Test Plan: Added, removed and set these values via bulk edit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T8740

Differential Revision: https://secure.phabricator.com/D18868
2018-01-19 12:47:10 -08:00
epriestley
687fada5af Restore bulk edit support for remarkup fields (description, add comment)
Summary:
Depends on D18866. Ref T13025. Fixes T12415. This makes the old "Add Comment" action work, and adds support for a new "Set description to" action (possibly, I could imagine "append description" being useful some day, maybe).

The implementation is just a `<textarea />`, not a whole fancy remarkup box with `[Bold] [Italic] ...` buttons, preview, typeaheads, etc. It would be nice to enrich this eventually but doing the rendering in pure JS is currently very involved.

This requires a little bit of gymnastics to get the transaction populated properly, and adds some extra validation since we need some code there anyway.

Test Plan:
  - Changed the description of a task via bulk editor.
  - Added a comment to a task via bulk editor.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T12415

Differential Revision: https://secure.phabricator.com/D18867
2018-01-19 12:45:34 -08:00
epriestley
bf1ac701c3 Support "select" types in bulk editor (status, priority)
Summary: Depends on D18864. Ref T13025. Adds bulk edit support back for "status" and "priority" using `<select />` controls.

Test Plan:
Used bulk editor to change status and priority for tasks.

{F5374436}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18866
2018-01-19 12:44:48 -08:00
epriestley
a251db4618 Remove the Maniphest-specific bulk job type
Summary: Depends on D18863. Ref PHI173. Ref T13025. After D18863, this job type is no longer used: the workflow uses a genric worker instead which can apply transactions to any object.

Test Plan: Grepped for callsites, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18864
2018-01-19 12:44:16 -08:00
epriestley
09e71a4082 Define bulk edits in terms of EditEngine, not hard-coded ad-hoc definitions
Summary:
Depends on D18862. See PHI173. Ref T13025. Fixes T10005. This redefines bulk edits in terms of EditEngine fields, rather than hard-coding the whole thing.

Only text fields -- and, specifically, only the "Title" field -- are supported after this change. Followup changes will add more bulk edit parameter types and broader field support.

However, the title field now works without any Maniphest-specific code, outside of the small amount of binding code in the `ManiphestBulkEditor` subclass.

Test Plan: Used the bulk edit workflow to change the titles of tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T10005

Differential Revision: https://secure.phabricator.com/D18863
2018-01-19 12:43:47 -08:00
epriestley
6ef45d8245 Provide a generic transaction-oriented bulk job worker
Summary:
Depends on D18806. Ref T13025. See PHI173. Currently, Maniphest bulk edits are processed by a Maniphest-specific worker. I want to replace this with a generic worker which can apply transactional edits to any object.

This implements a generic worker, although it has no callers yet. Future changes give it callers, and later remove the Maniphest-specific worker.

Test Plan: See next changes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18862
2018-01-19 12:41:56 -08:00
epriestley
7f91c8c4ac Rebuild the bulk editor on SearchEngine
Summary:
Depends on D18805. Ref T13025. Fixes T10268.

Instead of using a list of IDs for the bulk editor, power it with SearchEngine queries. This gives us the full power of SearchEngine and lets us use a query key instead of a list of 20,000 IDs to avoid issues with URL lengths.

Also, split it into a base `BulkEngine` and per-application subclasses. This moves us toward T10005 and universal support for bulk operations.

Also:

  - Renames most of "batch" to "bulk": we're curently inconsitent about this, I like "bulk" better since I think it's more clear if you don't regularly interact with `.bat` files, and newer stuff mostly uses "bulk".
  - When objects in the result set can't be edited because you don't have permission, show the status more clearly.

This probably breaks some stuff a bit since I refactored so heavily, but it seems mostly OK from poking around. I'll clean up anything I missed in followups to deal with remaining items on T13025.

Test Plan:
{F5302300}

  - Bulk edited from Maniphest.
  - Bulk edited from a workboard (no more giant `?ids=....` in the URL).
  - Hit most of the error conditions, I think?
  - Clicked the "Cancel" button.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T10268

Differential Revision: https://secure.phabricator.com/D18806
2018-01-19 12:40:08 -08:00
epriestley
ad659627b3 Make bulk editor working set editable and more homogenous
Summary:
Ref T13025. See PHI50. Fixes T11286. Ref T10005. Begin modernizing the bulk editor.

For T10005 ("move the bulk editor to modern infrastructure"), rewrite the rendering of the editable set so that it is application-agnostic and can work with any kind of object.

For T11286 ("let users de-select items in the working set"), make the working set editable.

Test Plan:
{F5302158}

  - Deselected some objects, applied an edit, saw the edit apply to only selected objects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T11286, T10005

Differential Revision: https://secure.phabricator.com/D18805
2018-01-19 12:39:27 -08:00
epriestley
3e983b583d Fix a transposed feed story in Badges
Summary: See <https://discourse.phabricator-community.org/t/badge-quality-from-and-to-interchanged-in-activity-log/987>. These are swapped.

Test Plan: Changed a badge quality, viewed feed story, saw it position the strings correctly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18870
2018-01-16 13:57:01 -08:00
epriestley
82bfb98179 Fix a copy/paste error on the burnup chart
Summary: See PHI286, maybe. See PHI273. Strongly considering just deleting this.

Test Plan: ~.~

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18865
2018-01-11 07:07:46 -08:00
Alex Vandiver
2140741e25 Fix typo in new setting description
Summary:
Noticed by @amckinley in
https://secure.phabricator.com/D18850#inline-57246 but not fixed
before landing.

Test Plan: ispell

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, amckinley, epriestley

Differential Revision: https://secure.phabricator.com/D18861
2018-01-06 07:25:27 -08:00
epriestley
adbd7d4fd8 Make the new synthetic burnup chart data respect the "Project" filter
Summary: See PHI273. Third time's the charm? This page has a "Project" filter which lets you view data for only one project, but the synthetic data currently ignores it.

Test Plan: Filtered burnup chart by various projects, saw sensible-looking data.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18860
2018-01-05 10:34:45 -08:00
epriestley
5543592034 Add a couple of clarifying comments to the Mercurial protocol parser
Summary: See D18857. Ref T13036. See PHI275. Explain what's going on here a little better since it isn't entirely obvious and debugging these stream parsers is a gigantic pain.

Test Plan: Read text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18859
2018-01-04 14:23:28 -08:00
epriestley
94db95a165 Sort burnup data chronologically after merging synthetic and "real" data
Summary:
Ref T13020. See PHI273. See D18853. On `secure`, the chart looks less promising than it did locally, and is full of discontinuities:

{F5356544}

I think this is a sorting issue. But if I can't fake my way through this soon I'll maybe get the Fact engine running and use it to provide the data here, as a sort of half-step toward T1562?

Test Plan: Chart looks the same locally, will push and see if `secure` improves?

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13020

Differential Revision: https://secure.phabricator.com/D18854
2018-01-04 14:08:03 -08:00
epriestley
13c8963dab Fix a Mercurial wire protocol parser issue when we receive a length frame before any data
Summary:
Depends on D18856. Ref T13036. See PHI275. When we receive a length frame but the buffer doesn't have any data yet, we currently emit a pointless 0-length data frame on the channel.

For normal chatter this is harmless/valid, but it causes problems when a channel has transitioned into bundle2 mode (probably it indicates "end of stream")?

In any case, it's never helpful, so if we're about to read a data block and don't have any data, just bail out until we see some more data.

Note that we can't end up here //expecting// a 0-length data block: both the `data-length` and `data-bytes` states already handle that properly.

Test Plan: Pushed 4MB changes to a Mercurial repository with Mercurial 4.1.1, was no longer able to hit channel errors.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18857
2018-01-04 14:06:52 -08:00
epriestley
3a4e14431f Remove an obsolete comment about Mercurial SSH error behavior
Summary:
Depends on D18855. Ref T13036. This comment no longer seems to be accurate: anything we send over `stderr` is faithfully shown to the user with recent clients.

From [[ https://www.mercurial-scm.org/repo/hg/file/default/mercurial/help/internals/wireprotocol.txt | this document ]], the missing sauce may have been:

```
A generic error response type is also supported. It consists of a an error
message written to ``stderr`` followed by ``\n-\n``. In addition, ``\n`` is
written to ``stdout``.
```

That is, writing "\n" to stdout in addition to writing the error to stderr. However, this no longer appears to be necessary.

I think the modern client behavior is generally sensible (and consistent with the behavior of Git and Subversion) so this //probably// isn't a bug or me making a mistake.

Test Plan: With a modern client, threw some arbitrary exception during execution. Observed a helpful message on the client with no additional steps.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18856
2018-01-04 14:05:44 -08:00
epriestley
0f02d79ffa Remove nonfunctional Mercurial "bundle2" capability filtering from SSH pathway
Summary:
Ref T13036. This code attempts to filter the "capabilities" message to remove "bundle2", but I think this has never worked.

Specifically, the //write// pathway is hooked, and "write" here means "client is writing a message to the server". However, the "capabilities" frame is part of the response, not part of the request. Thus, this code never fires, at least on recent versions of Mercurial.

Since I plan to support bundle2 and don't want to decode response frames, just get rid of this, assuming we'll achieve those goals.

I think this was just overlooked in D14241, which probably focused on the HTTP version. This code does (at least, potentially) do something for HTTP.

I'm leaving the actual "strip stuff" code in place for now since I think it's still used on the HTTP pathway.

Test Plan:
  - Added debug logging, saw this code never hit even though `hg push --debug` shows the client believing bundle2 is supported.
  - Logged both halves of the wire protocol and saw this come from the server, not the client.
  - Ran the failing `hg push` of a 4MB file under hg 4.4.1, got the same error as before.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: cspeckmim

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18855
2018-01-04 14:05:13 -08:00
epriestley
f3f1f9dc57 Allow "drydock.blueprint.edit" to create blueprints
Summary:
Depends on D18848. Ref PHI243. This puts a bit of logic up front to figure out the blueprint type before we actually start editing it.

This implementation is a little messy but it keeps the API clean. Eventually, the implementation could probably go in the TransactionTypes so more code is shared, but I'd like to wait for a couple more of these first.

This capability probably isn't too useful, but just pays down a bit of technical debt from the caveat introduced in D18822.

Test Plan:
  - Created a new blueprint with the API.
  - Tried to create a blueprint without a "type" (got a helpful error).
  - Created and edited blueprints via the web UI.
  - Tried to change the "type" of an existing blueprint (got a helpful error).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18849
2018-01-04 10:08:07 -08:00
epriestley
6d9776fa89 Give EditEngine a Conduit-specific initialization pathway for objects
Summary:
Depends on D18845. See PHI243 for context and more details.

Briefly, some objects need a "type" transaction (or something similar) very early on, and we can't generate their fields until we know the object type. Drydock blueprints are an example: a blueprint's fields depend on the blueprint's type.

In web interfaces, the workflow just forces the user to select a type first. For Conduit workflows, I think the cleanest approach is to proactively extract and apply type information before processing the request. This will make the implementation a little messier, but the API cleaner.

An alternative is to add more fields to the API, like a "type" field. This makes the implementation cleaner, but the API messier. I think we're better off favoring a cleaner API here.

This change just makes it possible for `DrydockBlueprintEditEngine` to look at the incoming transactions and extract a "type"; it doesn't actually change any behavior.

Test Plan: Performed edits via API, but this change doesn't alter any behavior.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18847
2018-01-04 10:07:07 -08:00
epriestley
83c528c464 Modularize transactions for Drydock Blueprints
Summary: Ref PHI243. This is a followup to D18822, which added an edit-only `drydock.blueprint.edit`. By modularizing transactions (here) and then adding a "type" transaction (next change) I intend to remove the "edit-only" limitation and make this API method fully functional.

Test Plan: Created and edited blueprints via the web UI. Edited blueprints via the API. Disabled/enabled blueprints (currently web UI only).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18845
2018-01-04 10:03:44 -08:00
epriestley
53b25db918 Prevent enormous changes from being pushed to repositoires by default
Summary:
Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes".

If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe.

Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default.

Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See <https://discourse.phabricator-community.org/t/herald-enormous-check/822>.

Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: joshuaspence

Maniphest Tasks: T13031

Differential Revision: https://secure.phabricator.com/D18850
2018-01-04 10:02:29 -08:00
epriestley
cb957f8d62 Pile more atrocities onto the Maniphest burnup report
Summary:
See PHI273. Ref T13020. After D18777, tasks created directly into the default status (which is common) via the web UI no longer write a "status" transaction.

This is consistent with other applications, and consistent with the API/email behavior for tasks since early 2016. It also improves the consistency of //reading// tasks via the API.

However, it impacted the "Burnup Report" which relies on directly reading these rows to detect task creation. Until this is fixed properly (T1562), synthetically generate the "missing" transactions which this page expects by looking at task creation dates instead.

Specifically, we:

  - Generate a fake `status: null -> "open"` transaction for every task by looking at the Task table.
  - Go through the transaction list and remove all the legacy `status: null -> "any open status"` transactions. These will only exist for older tasks.
  - Merge all our new fake transactions into the list of transactions.
  - Continue on as though nothing happened, letting the rendering code continue to operate on legacy-looking data.

I think this will slightly miscount tasks which were created directly into a closed status, but this is very rare, and does not significantly impact the accuracy of this report relative to other known issues (notably, merging closed tasks).

This will also get the wrong result if the default status has changed from an "open" status to a "closed" status at any point, but this is exceptionally bizarre/rare.

Ultimately, T1562 will let us delete all this stuff and disavow its existence.

Test Plan:
  - Created some tasks, loaded burnup before/after this patch.
  - My local chart looks more accurate afterwards, but the data is super weird (I used `bin/lipsum` to create a huge number of tasks a couple months ago). I'll vet this on `secure`, which has more reasonable data.

Here's my local chart:

{F5356499}

That's what it //should// look like, it's just hard to be confident that nothing else is hiding there.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13020

Differential Revision: https://secure.phabricator.com/D18853
2018-01-04 10:02:15 -08:00
epriestley
129e3a1208 Fix a minor/harmless race with feed publishers in certain draft states
Summary:
Depends on D18851. Ref T13035. After D18819, revision creation transactions may be split into two groups (if prototypes are enabled).

This split means we have two workers. The first worker doesn't publish feed stories or mail; the second one does.

Currently, both workers call `shouldPublishFeedStory()` before they queue, and then again after the daemons pull them out of the queue. However, the answer to this question can change.

Specifically, this happens:

  - `arc` creates a revision.
  - The first transaction group applies, creating the revision as a draft, and returns `false` from `shouldPublishFeedStory()`, and does not generate related PHIDs. It queues a daemon to send mail, expecting it not to publish a feed story.
  - The second transaction group applies, promoting the revision to "needs review". Since the revision has promoted, `shouldPublishFeedStory()` now returns true. This editor generates related PHIDs and queues a daemon task, expecting it to send mail / publish feed.
  - A few milliseconds pass.
  - The first job gets pulled out of the daemon queue and worked on. It does not have any feed metadata because the object wasn't publishable when the job was queued -- but `shouldPublishFeedStory()` now returns true, so it tries to publish a story without any metadata available. Slightly bad stuff happens (see below).
  - The second job gets pulled out of the daemon queue and worked on. This one has metadata and works fine.

The "slightly bad stuff" is that we publish an empty feed story with no references to any objects, then try to push it to hooks and other listeners. Since it doesn't have any references, it fails to load during the "push to external listeners" phase.

This is harmless but clutters the log and doesn't help anything.

Instead, cache the state of "are we publishing a feed story for this object?" when we queue the worker so it can't race.

Test Plan:
  - Enabled prototypes.
  - Disabled all Herald triggers for Harbormaster build plans.
  - Ran `bin/phd debug task` in one window.
  - Created a revision in a separate window.
  - Before patch: saw "unable to load feed story" errors in the daemon log.
  - After patch: no more "unable to load feed story" errors.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13035

Differential Revision: https://secure.phabricator.com/D18852
2018-01-04 08:14:55 -08:00
epriestley
de6c68b91e Always show "X requested review" in mail to stop some undraft mail from being dropped
Summary: Ref T13035. See that task for a description of the issue.

Test Plan:
  - Enabled prototypes.
  - Disabled all Herald rules that trigger Harbormaster builds.
  - Created a new revision.
  - Before patch: initial review request email was dropped.
  - After patch: initial review request email is sent.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13035

Differential Revision: https://secure.phabricator.com/D18851
2018-01-04 08:14:31 -08:00
epriestley
ead5f4fd9c Add an "Accepting reviewers" Herald field for commits
Summary:
See PHI262. Fixes T12578. Although this is a bit niche and probably better accomplished through advisory/soft measures ("Add blocking reviewers") in most cases, it isn't difficult to implement and doesn't create any technical or product tension.

If installs write a rule that blocks commits, that will probably also naturally lead them to an "add reviewers" rule anyway.

Also, allow packages to be hit with the typeahead. They're valid reviewers but previously you couldn't write rules against them, for no actual reason.

Test Plan: Used test console to run this against commits, got sensible results for the field value.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12578

Differential Revision: https://secure.phabricator.com/D18839
2017-12-26 15:59:36 -08:00
epriestley
ad4db9b2f3 Separate "Set/Reset Password" from "Change Password"
Summary:
See PHI223. Ref T13024. There's a remaining registration/login order issue after the other changes in T13024: we lose track of the current URI when we go through the MFA flow, so we can lose "Set Password" at the end of the flow.

Specifically, the flow goes like this today:

  - User clicks the welcome link in email.
  - They get redirected to the "set password" settings panel.
  - This gets pre-empted by Legalpad (although we'll potentially survive this with the URI intact).
  - This also gets pre-empted by the "Set MFA" workflow. If the user completes this flow, they get redirected to a `/auth/multifactor/?id=123` sort of URI to highlight the factor they added. This causes us to lose the `/settings/panel/password/blah/blah?key=xyz` URI.

The ordering on this is also not ideal; it's preferable to start with a password, then do the other steps, so the user can return to the flow more easily if they are interrupted.

Resolve this by separating the "change your password" and "set/reset your password" flows onto two different pages. This copy/pastes a bit of code, but both flows end up simpler so it feels reasonable to me overall.

We don't require a full session for "set/reset password" (so you can do it if you don't have MFA/legalpad yet) and do it first.

This works better and is broadly simpler for users.

Test Plan:
  - Required MFA + legalpad, invited a user via email, registered.
    - Before: password set flow got lost when setting MFA.
    - After: prompted to set password, then sign documents, then set up MFA.
  - Reset password (with MFA confgiured, was required to MFA first).
  - Tried to reset password without a valid reset key, wasn't successful.
  - Changed password using existing flow.
  - Hit various (all?) error cases (short password, common password, mismatch, missing password, etc).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18840
2017-12-26 08:34:14 -08:00
epriestley
66b74073be Provide ANSI color information for Harbormaster build status via API
Summary:
Ref PHI261. This moves Harbormaster build status to work more similarly to other modern status types, like Differential revision status, where we try to specify as much behavior on the server as possible so that the client and server can vary independently.

(I don't have any specific plans to make Harbormaster build status configurable on the server side, but it isn't out of the question.)

Test Plan: Ran `harbormaster.querybuilds` (saw same data as before) and `harbormaster.build.search` (saw same data as before, plus new ANSI color data).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18838
2017-12-23 11:39:05 -08:00
Mukunda Modell
bd5aa0c90f Prevent hiding the PhabricatorProjectDetailsProfileMenuItem
Summary: This probably isn't the best solution, however, it conveniently avoids the bug from T13033. It would probably be more user-friendly (but more difficult to implement) if we allowed either Project Details //or// Workboard to be hidden but not both.

Test Plan: Tested locally, indeed this prevents hiding the menu item.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18836
2017-12-23 11:38:05 -08:00
epriestley
84cf493879 Allow "Manage" to be the default menu item for projects
Summary:
Fixes T13033. Currently (prior to D18836) all the default items for projects can be hidden. When this occurs, the main project page fatals.

If we fix the fatal narrowly (don't try to call `null->getBuiltinKey()` when `$default` is `null`), it would 404, which is a little better but not by much.

After D18836 you can't hide "Profile", which is pretty sensible, and effectively fixes this. This change doubles down: let "Manage" be a default, and send the user there if we can't find a different default.

Ideally, the MenuEngine itself will do more rendering eventually (as it does for some of the newer Home stuff) and could handle this defaulting behavior with less special casing, but we'd still end up in a similar situation if a project had only one "Link" item to "coolcats.com" or something: redirecting the user to "coolcats.com" is probably better than fataling, but not by a huge margin, and not likely to be what they expect.

Test Plan:
Before D18836, disabled both "Profile" and "Workboard" items on a project. Visited project page.

Before patch: fatal. After patch: manage page.

After D18836, you can't do this and just get the profile, so this is sort of moot and mostly future-proofing/for-completeness.

Reviewers: 20after4, amckinley

Reviewed By: amckinley

Maniphest Tasks: T13033

Differential Revision: https://secure.phabricator.com/D18843
2017-12-23 11:37:12 -08:00
Dmitri Iouchtchenko
393824656f Don't notify without notifiable attendees
Summary: Events with no attendees (e.g. fresh instances of recurring events) would trigger an exception when sending notifications, because `$attendee_map` would still get populated.

Test Plan: Declined event, restarted daemons. Did not see exception. Accepted event, restarted daemons. Saw "[Calendar] [Reminder]" email.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18835
2017-12-21 12:46:46 -08:00
epriestley
e411d75964 Fix an issue where blame could fatal for unrecognized authors
Summary: See PHI255. See <https://discourse.phabricator-community.org/t/error-generating-blame-data/766>.

Test Plan:
  - Viewed a file contributed to by users with no Phabricator user accounts, in Diffusion.
  - Enabled blame.
  - Before patch: blame failed, fatal in logs.
  - After patch: blame worked.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18833
2017-12-20 11:20:23 -08:00
epriestley
8dccf05c4c Manually set "max_allowed_packet" to 1GB for "mysqldump"
Summary:
We have one production instance with failing database backups since they recently uploaded a 52MB hunk. The production configuration specifies a 64MB "max_allowed_packet" in `[mysqld]`, but this doesn't apply to `mysqldump` (we'd need to specify it in a separate `[mysqldump]` section) and `mysqldump` runs with an effective limit of the default (16MB).

We could change our production config to specify a value in `[mysqldump]`, but just change it unconditionally at execution time since there's no reason for any user to ever want this command to fail because they have too much data.

Test Plan: Dumped locally, will verify production backup goes through cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18834
2017-12-20 10:29:02 -08:00
epriestley
7cbbe2ccf7 When users browse to a submodule path in Diffusion explicitly, don't fatal
Summary: Ref T13030. See PHI254. This behavior could be cleaner than I've made it, but it fixes the "this is totally broken" issue, replacing a fatal/exception with an informative (just not terribly useful) page.

Test Plan:
  - Added a submodule to a repository.
  - In Diffusion, clicked some other file next to the submodule, then edited the URI to the submodule path instead.
    - Before patch: fatal.
    - After patch: relatively useful message about this being a submodule.

Note that it's normally hard to hit this URI directly. In the browse view, submodules are marked up as directories and linked to a separate submodule resolution flow.

{F5321524}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13030

Differential Revision: https://secure.phabricator.com/D18831
2017-12-18 09:18:22 -08:00
epriestley
a1ad184ddd Denormalize added and removed line counts for the current diff onto revisions
Summary:
See PHI230. Currently, we denormalize raw line counts onto diffs and revisions, but not added/removed line counts.

I'd like to try a `[---+  ]` sort of size hint element (see D16322 for more) as a general approach to conveying size information at a glance and see how it feels, since I think the raw size number isn't very scannable/useful and it may be a significant improvement to hint about how much of a change is throwing stuff out vs adding new stuff.

This just makes the data available without any subquerying and doesn't actually change the UI.

Test Plan:
Created a revision, saw detailed change information populate in the database.

```
mysql> select * from differential_revision where id = 292\G
*************************** 1. row ***************************
              id: 292
           title: WIP
   originalTitle: WIP
            phid: PHID-DREV-ux3cxptibn3l5pxsug3z
          status: draft
         summary: asdf
        testPlan: asdf
      authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
lastReviewerPHID: NULL
       lineCount: 41
     dateCreated: 1513179418
    dateModified: 1513179418
        attached: []
         mailKey: h4mn6perdio47o4beomyvu75zezwvredx3mbrlgz
      branchName: NULL
      viewPolicy: users
      editPolicy: users
  repositoryPHID: PHID-REPO-wif5lutk5gn3y6ursk4p
      properties: {"lines.added":40,"lines.removed":1}
  activeDiffPHID: PHID-DIFF-ixjphpunpkenqgukpmce
1 row in set (0.00 sec)
```

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18832
2017-12-18 09:17:55 -08:00
epriestley
e1d6bad864 Stop trying to assess the image dimensions of large files and file chunks
Summary:
Depends on D18828. Ref T7789. See <https://discourse.phabricator-community.org/t/git-lfs-fails-with-large-images/584>.

Currently, when you upload a large (>4MB) image, we may try to assess the dimensions for the image and for each individual chunk.

At best, this is slow and not useful. At worst, it fatals or consumes a ton of memory and I/O we don't need to be using.

Instead:

  - Don't try to assess dimensions for chunked files.
  - Don't try to assess dimensions for the chunks themselves.
  - Squelch errors for bad data, etc., that `gd` can't actually read, since we recover sensibly.

Test Plan:
  - Created a 2048x2048 PNG in Photoshop using the "Random Noise" filter which weighs 8.5MB.
  - Uploaded it.
  - Before patch: got complaints in log about `imagecreatefromstring()` failing, although the actual upload went OK in my environment.
  - After patch: clean log, no attempt to detect the size of a big image.
  - Also uploaded a small image, got dimensions detected properly still.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18830
2017-12-18 09:17:32 -08:00
epriestley
5295840a4c Restore the "Download from Git LFS" UI button to Diffusion
Summary: Depends on D18827. Ref T7789. See PHI204. See PHI131. This button got accidentally removed in Diffusion refactoring (`$data` is no longer used).

Test Plan: {F5321459}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18828
2017-12-18 09:13:19 -08:00
epriestley
8e416474c0 Add a Herald pre-commit field for detecting LFS usage
Summary: Depends on D18825. Ref T7789. See PHI131. Allows installs to selectively disable LFS by adding Herald rules to block commits that use LFS.

Test Plan:
  - Wrote an LFS rule ("When commit uses git lfs, block commit").
  - Pushed an LFS commit: rejected.
  - Pushed a non-lFS commit: success.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18827
2017-12-18 09:12:52 -08:00
epriestley
e34b4bbd90 Move the Git LFS gate to dedicated (non-prototype) config
Summary: See PHI131. Ref T7789. Although this probably isn't 100% complete, there don't seem to be any actual, known, practical blocking issues remaining (everything is either heresay or not reproducible).

Test Plan: Tried to push LFS locally, got blocked with a helpful message. Enabled setting, tried to push LFS locally, got a successful push.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18825
2017-12-18 09:12:22 -08:00
epriestley
c9a0d68340 Allow Herald rules to add comments
Summary:
See PHI242. All use cases for this that I know of are pretty hacky, but they don't seem perilous, and it's easier than webhooks.

See P1895, T10183, and T9853 for me previously refusing to implement this since all those use cases were also pretty bad.

Test Plan:
  - Wrote a rule to add comments, saw it add comments.
  - Reviewed summary, re-edited rule, reviewed transcript to check that all the strings worked OK.
  - Wrote a new rule for a non-commentable object (a blog) to make sure I wasn't offered the "Add a comment" action.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18823
2017-12-18 09:10:57 -08:00
Tim Hirsh
60e5c0ec1b Add drydock.blueprint.edit Conduit method
Summary:
Ref: https://admin.phacility.com/PHI243

Since our use case primarily focuses on transaction editing, this patch implements the `drydock.blueprint.edit` api method with the understanding that:
a) this is a work in progress
b) object editing is supported, but object creation is not yet implemented

Test Plan:
* updated existing blueprints via Conduit UI
* regression tested `maniphest.edit` by creating new and updating existing tasks

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, yelirekim, jcox

Differential Revision: https://secure.phabricator.com/D18822
2017-12-08 11:55:08 -05:00
epriestley
6d3baa92f9 Execute Herald again after promoting revisions out of the "Draft" state
Summary:
Fixes T13027. Ref T2543. When revisions promote from "Draft" because builds finish or no builds are configured, the status currently switches from "Draft" to "Needs Review" without re-running Herald.

This means that some rules -- notably, "Send me an email" rules -- don't fire as soon as they should.

Instead of applying this promotion in a hacky way inline, queue it and apply it normally in a second edit, after the current group finishes.

Test Plan:
  - Created a revision, reviewed Herald transcripts.
  - Saw three Herald passes:
    - First pass (revision creation) triggered builds and no email.
    - Second pass (builds finished) did not trigger builds (no update) and did not trigger email (revision still a draft).
    - Third pass (after promotion out of 'draft') did not trigger builds (no update) but did trigger email (revision no longer a draft).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13027, T2543

Differential Revision: https://secure.phabricator.com/D18819
2017-12-05 12:13:56 -08:00
epriestley
4f8340c05f Restore the "Log In" menubar action
Summary:
See <https://discourse.phabricator-community.org/t/activation-link-in-welcome-mail-only-works-if-new-user-isnt-semi-logged-in/740/7>.

In T13024, I rewrote the main menu bar to hide potentially sensitive items (like notification and message counts and saved search filters) until users fully log in.

However, the "Log In" item got caught in this too. For clarity, rename `shouldAllowPartialSessions()` to `shouldRequireFullSession()` (since logged-out users don't have any session at all, so it would be a bit misleading to say that "Log In" "allows" a partial session). Then let "Log In" work again for logged-out users.

(In most cases, users are prompted to log in when they take an action which requires them to be logged in -- like creating or editing an object, or adding comments -- so this item doesn't really need to exist. However, it aligns better with user expectations in many cases to have it present, and some reasonable operations like "Check if I have notifications/messages" don't have an obvious thing to click otherwise.)

Test Plan: Viewed site in an incognito window, saw "Log In" button again. Browsed normally, saw normal menu.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18818
2017-12-05 12:13:10 -08:00
epriestley
a989dd181d Fix Mercurial commit history ordering
Summary:
See <https://discourse.phabricator-community.org/t/diffusion-observed-mercurial-repository-history-broken/825>.

In D18769, I rewrote this from using the `--branch` flag (which is unsafe and does not function on branches named `--config=x.y` and such).

However, this rewrite accidentally changed the result order, which impacted Mercurial commit hisotry lists and graphs. Swap the order of the constraints so we get newest-to-oldest again, as expected.

Test Plan: Viewed a Mercurial repository's history graph, saw sensible chronology after the patch.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18817
2017-12-05 12:12:44 -08:00
lkassianik
46d496b8cc Fix error for URL's that could mean several commits
Summary: Ref T13001, URLs that return multiple commits should show a list of those commits. Not sure if the actual list looks very pretty this way, but was wondering if this approach was vaguely correct.

Test Plan:
- Navigate to `install/rPbd3c23`
- User should see a list view providing links to `install/rPbd3c2355e8e2b220ae5e3cbfe4a057c8088c6a38` and `install/rPbd3c239d5aada68a31db5742bbb8ec099074a561`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13001

Differential Revision: https://secure.phabricator.com/D18816
2017-12-05 19:24:57 +00:00
epriestley
c924351a58 Mark sessions as "signed all documents" when Legalpad has been uninstalled
See PHI238. When an install uninstalls "Legalpad", we were incorrectly failing
to mark sessions as "Signed All Required Documents" by bailing early.

Test Plan: Uninstalled Legalpad, logged in.
2017-12-02 16:15:59 -08:00
lkassianik
42034e6739 Property list view on Diffusion commits should show build status but not Subscriptions, Projects, or Tokens
Summary: Ref T13019, adds build status back to Diffusion commits

Test Plan: Open a Diffusion commit that has a build status, property list view should show the build status, but not Subscriptions, Projects, or Tokens.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13019

Differential Revision: https://secure.phabricator.com/D18813
2017-12-01 18:16:26 +00:00
epriestley
5240cffd9c Fix an issue where Diffusion could fatal if the default branch was deleted
Summary: See PHI234. In T12931 we improved the behavior of Diffusion when a repository's default branch is set to a branch that does not exist, but in T11823 the way refcursors work changed, and we can now get a cursor (just with no positions) back for a deleted branch. When we did, we didn't handle things gracefully.

Test Plan:
  - Set default branch to a deleted branch, saw nice error instead of fatal.
  - Set default branch to a nonexistent branch which never existed, saw nice error.
  - Set default branch to existing "master", saw repository normally.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18811
2017-11-30 14:06:41 -08:00
epriestley
14cc0abeb3 Fix several safety issues with repository URIs
Summary:
See PHI234. Several issues here:

  - The warning about observing a repository in Read/Write mode checks the raw I/O type, not the effective I/O type. That means we can fail to warn if other URIs are set to "Default", and "Default" is "Read/Write" in practice.
  - There's just an actual typo which prevents the "Observe" version of this error from triggering properly.

Additionally, add more forceful warnings that "Observe" and "Mirror" mean that you want to //replace// a repository with another one, not that we somehow merge branches selectively. It isn't necessarily obvious that "Observe" doesn't mean "merge/union", since the reasons it can't in the general case are somewhat subtle (conflicts between refs with the same names, detecting ref deletion).

Test Plan:
Read documentation. Hit the error locally by trying to "Observe" while in Read/Write mode:

{F5302655}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18810
2017-11-30 14:06:21 -08:00
epriestley
f786c86a6a Don't require the "gd" extension be installed in order to run unit tests
Summary:
Ref T10405. If `gd` is not installed, a number of unit tests currently fail because they generate synthetic users and try to composite profile pictures for them.

Instead, just fall back to a static default if `gd` is not available.

Test Plan: Faked this pathway, created a new user, saw a default profile picture.

Reviewers: amckinley, lpriestley, avivey

Reviewed By: avivey

Maniphest Tasks: T10405

Differential Revision: https://secure.phabricator.com/D18812
2017-11-30 13:51:31 -08:00
epriestley
0807b70ea1 Add an explicit warning in the Differential transaction log when users skip review
Summary:
Ref T10233. See PHI231. When users ignore the `arc land` prompt about bad revision states, make it explicitly clear in the transaction log that they broke the rules.

You can currently figure this out by noticing that there's no "This revision is accepted and ready to land." message, but it's unrealistic to expect non-expert users to look for the //absence// of a message to indicate something, and this state change is often relevant.

Test Plan: {F5302351}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10233

Differential Revision: https://secure.phabricator.com/D18808
2017-11-30 11:03:55 -08:00
Aviv Eyal
d8f2630d5c Modernize QuickSearch typeahead
Summary:
Use ClassQuery to find datasources for the quick-search.

Mostly, this allows extensions to add quicksearches.

Test Plan:
using `/typeahead/class/`, tested several search terms that make sense.
Removed the tag interface from a datasource, which removed it from results.

Reviewers: epriestley, amckinley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18760
2017-11-30 15:07:49 +00:00
epriestley
1b76250f89 Disallow "Accept" on drafts, allow "Resign"
Summary:
Depends on D18801. Ref T2543. See PHI229. I missed "Accept" before, but intended to disallow it (like "Reject") since I don't want drafts to be reviewable.

However, "Resign" seems fine to allow? So let's allow that for now.

Test Plan: Was no longer offered "Accept" on draft revisions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18802
2017-11-28 13:46:26 -08:00
epriestley
54f131dc4b Make "first broadcast" rules for Differential drafts more general
Summary:
See PHI228. Ref T2543. The current logic gets this slightly wrong: prototypes are off, you create a draft with `--draft`, then promote it with "Request Review". This misses both branches.

Instead, test these conditions a little more broadly. We also need to store broadcast state since `getIsNewObject()` isn't good enough with this workflow.

Test Plan:
  - With prototypes on and autopromotion, got a rich email after builds finished.
  - With prototypes off, got a rich email immediately.
  - With prototypes off and `--draft`, got a rich email after "Request Review".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18801
2017-11-28 13:42:10 -08:00
epriestley
c7d6fd198c Support "Set X to" as an action in Herald for tokenizer/datasource custom fields
Summary:
See PHI173. Adds custom field support for Herald actions, and implements actions for "Datasource/Tokenizer" fields.

The only action available for now is "set field to...". Other actions ("Add values", "Remove values") might make sense in the future for these fields, but there's currently no use case. For most other field types (text, select, checkbox, etc) only "Set to" makes sense.

Test Plan:
  - Added a "datasource" custom field to the custom field definition in Config.
  - Added a "if field is empty, set field to default value X" rule to Herald.
  - Created a task with a nonempty field: no Herald trigger.
  - Created a task with an empty field: Herald fired.
  - Reviewed rule and transcripts for text strings.

{F5297615}

{F5297616}

{F5297617}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18784
2017-11-28 13:41:52 -08:00
epriestley
e919233b31 Don't show personalized menu items until users establish a full session
Summary:
Depends on D18792. Fixes T13024. Fixes T89198. Currently, when users are logging in initially (for example, need to enter MFA) we show more menu items than we should.

Notably, we may show some personalized/private account details, like the number of unread notifications (probably not relevant) or a user's saved queries (possibly sensitive). At best these are misleading (they won't work yet) and there's an outside possibility they leak a little bit of private data.

Instead, nuke everything except "Log Out" when users have partial sessions.

Test Plan:
Hit a partial session (MFA required, email verification required) and looked at the menu. Only saw "Log Out".

{F5297713}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18793
2017-11-28 10:01:58 -08:00
epriestley
dc62d18b47 Allow MFA enrollment before email verification
Summary: Depends on D18791. Ref T13024. This clears up another initialization order issue, where an unverified address could prevent MFA enrollment.

Test Plan: Configured both verification required and MFA required, clicked "Add Factor", got a dialog for the workflow.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18792
2017-11-28 10:01:09 -08:00
epriestley
ab0f61aa32 Tell users to "Wait Patiently" for admin account verification later in the registration process
Summary:
Depends on D18790. Ref T13024. Fixes T8335. Currently, "unapproved" and "disabled" users are bundled together. This prevents users from completing some registration steps (verification, legalpad documents, MFA enrollment) before approval.

Separate approval out and move it to the end so users can do all the required enrollment stuff on their end before we roadblock them.

Test Plan: Required approval, email verification, signatures, and MFA. Registered an account. Verified email, signed documents, enrolled in MFA, and then got prompted to wait for approval.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024, T8335

Differential Revision: https://secure.phabricator.com/D18791
2017-11-28 10:00:03 -08:00
epriestley
c7718d3a21 Ask users to sign Legalpad documents before requiring they enroll in MFA
Summary:
Depends on D18789. Ref T13024. See PHI223. Currently, if `security.require-multi-factor-auth` and Legalpad "Signature Required" documents are //both// set, it's not possible to survive account registration, since MFA is requiried to sign and signatures are required to add MFA.

Instead, check for signatures before requiring MFA enrollment. This makes logical sense, since it's silly to add MFA if you don't agree to a Terms of Service or whatever.

(Note that if you already have MFA, we prompt for that first, before either of these steps, which also makes sense.)

Test Plan: Configured `security.require-multi-factor-auth`. Added a signature-required document. Loaded a page as a new user. Went through signature workflow, then through the MFA enrollment workflow.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18790
2017-11-28 09:59:39 -08:00
epriestley
e850bc6b95 When requiring login signatures, order documents from oldest to newest
Summary: Depends on D18788. Ref T13024. Currently, we prompt users to sign from newest to oldest. This seems less intuitive than oldest to newest.

Test Plan: Dumped document order, saw it swap to oldest-first.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18789
2017-11-28 09:59:22 -08:00
epriestley
ba4b9f7184 Refactor on-login Legalpad document signature requirement
Summary: Depends on D18786. Ref T13024. I'm going to change the order this occurs in, but move it to a separate method and clean it up a little first.

Test Plan: Added a new document as required, reloaded, signed it, got logged into a session.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18788
2017-11-28 09:58:53 -08:00
epriestley
71406ca93b Lightly modernize LegalpadDocumentSearchEngine
Summary: Depends on D18785. Ref T13024. While I'm in here, update this a bit to use the newer stuff.

Test Plan: Searched for Legalpad documents, saw more modern support (subscribers, order by, viewer()).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18786
2017-11-28 09:56:49 -08:00
epriestley
3d742eb50b Lightly modernize LegalpadDocumentQuery
Summary: Ref T13024. Updates LegalpadDocumentQuery to use slightly more modern constructions.

Test Plan: Queried for Legalpad documents, got the same results.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18785
2017-11-28 09:56:33 -08:00
epriestley
00baa3c1dd Hide archived projects only on workboards, not hovercards
Summary:
See PHI225. Previously, see D15335 / T10413. On workboards, we hide archived project tags since they aren't terribly useful in that context, at least most of the time. Originally, see T10349#159916 and D15297.

However, hovercards reuse this display logic, and it's inconsistent/confusing to hide them there, since the actual "Tags" elements on task pages show them. Narrow the scope of this rule.

Test Plan:
  - Viewed a hovercard for a task with an archived project tagged, saw archived project.
  - Viewed a workboard for the same task, saw only unarchived projects other than the current board tagged (this behavior is unchanged).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18783
2017-11-27 14:37:51 -08:00
epriestley
49b57eae7d Revert partial/nonfunctional OpenGraph support
Summary:
Ref T13018. See that task and the Discourse thread for discussion.

This doesn't work as-is and we need to `og:description` everything to make it work. I don't want to sink any more time into this so just back all the changes out for now.

(The `<html>` change is unnecessary anyway.)

Test Plan: Strict revert.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13018

Differential Revision: https://secure.phabricator.com/D18782
2017-11-22 15:21:10 -08:00
epriestley
c3d6c4b0ee Include OpenGraph prefix material in <html> tag if OpenGraph is enabled
Summary: Ref T13018. Discourse doesn't seem to be picking this up yet (see <https://discourse.phabricator-community.org/t/opengraph-test-topic/762>) so maybe it really needs this meta-meta-XML stuff?

Test Plan: Will push.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13018

Differential Revision: https://secure.phabricator.com/D18781
2017-11-22 11:37:08 -08:00
epriestley
2c72c2b924 Add basic support for OpenGraph header tags for public installs
Summary: Ref T13018. This is easy to get working roughly, at least, and seems reasonable.

Test Plan: Viewed page source, saw tags. Custom header logo still worked. Pretty hard to debug against a local install since Disqus / debugger tools can't hit it, but I'll see what it looks like in production and tweak it if I got anything horribly wrong.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13018

Differential Revision: https://secure.phabricator.com/D18780
2017-11-22 11:16:56 -08:00
epriestley
2d4a158356 Fix a bad link target in Diffusion content search results
Summary: See <https://discourse.phabricator-community.org/t/broken-links-in-diffusion-pattern-search-results-page/757>. This links to the display path, which is incorrect.

Test Plan:
  - In any repository, browsed into a directory.
  - Used pattern search to search for something that hits results.
  - Clicked the title (filename/path) of a result table.
    - Before patch: URL omits path context, 404 or wrong result.
    - After patch: taken to proper page.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18779
2017-11-22 11:16:14 -08:00
epriestley
d321cc810a Freeze "maniphest.gettasktransactions" and make status/priority transactions more consistent
Summary:
Ref T13020. See PHI221.

Freeze legacy method `maniphest.gettasktransactions` in favor of modern method `transaction.search`.

Remove legacy "null on create" behavior from Maniphest status and priority transactions. This behavior is obsolete with EditEngine, and leads to inconsistent transaction sets in the transaction record.

The desired behavior is that transactions which don't do anything (e.g., default value was not changed) don't appear in the transaction log.

Test Plan:
  - Viewed API UI and saw `maniphest.gettasktransactions` marked as "Frozen".
  - Created a new task via web UI (without changing status/priority), queried transactions with `maniphest.gettasktransacitons`/`transaction.search`, no longer saw "null on create" no-op transactions in record.
    - Web UI is unchanged, since these transactions were hidden before and now do not exist.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13020

Differential Revision: https://secure.phabricator.com/D18777
2017-11-22 11:13:53 -08:00
Austin McKinley
bea45e90d3 Add yaml files to differential.whitespace-matters
Summary: Whitespace has semantic meaning for yaml files, so we shouldn't suppress whitespace-only lines of diff by default.

Test Plan: Edited local config to include yaml files, saw expected whitespace changes.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18775
2017-11-15 11:57:11 -08:00
epriestley
3700bcb638 Warn and prevent 1-up/2-up switch in Differential if the user is editing an inline
Summary:
See PHI180. Currently, if you begin creating or editing an inline and then swap display modes (for example, with "View Unified"), your edit is lost.

Persisting the editor state is complicated and this is very rare, so just prevent the action and warn the user instead.

Also make the warning persist for a little longer since a few of the messages, including this one, take a couple seconds to read now.

Test Plan:
  - Edited a comment, tried to swap display modes, got a warning.
  - Swapped display modes normally with no comment being edited.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18774
2017-11-15 10:02:48 -08:00
Mukunda Modell
a1f12b4ac7 Specify a null behavior for the callsign sort column.
Summary:
Fixes paging on the Diffusion Repository List.

PhabricatorRepositoryQuery needs to specify a behavior for `null` on the OderableColumns definition for the `callsign` column.

See https://phabricator.wikimedia.org/T180457

Test Plan:
1. On an instance with more than 100 repositories
 * some of which are missing a callsign
2. Attempt to sort by callsign.
3. See the sorted results

Previously:

3. Exception: "Column "0" has null value, but does not specify a null behavior."

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18773
2017-11-14 17:16:01 -06:00
epriestley
95a5b41b0b Allow "arc diff --draft" to create revisions as drafts more forcefully
Summary:
Depends on D18771. See PHI206. Currently, `arc diff --draft` only holds revisions in draft mode: it doesn't put them into draft mode if the install isn't configured to use draft mode.

Instead, make it a bit more forceful so that `arc diff --draft` can create into draft mode explicitly even if protoypes are off. This aligns with expection a little more clearly.

Test Plan: Ran `arc diff --draft` with prototypes off, got a revision held in draft mode.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18772
2017-11-14 12:30:47 -08:00
epriestley
314e7266c3 Restore "Summary" and "Test Plan" to initial mail for non-draft configurations
Summary: See PHI210. Ref T2543. Currently, we don't set this flag if you have prototypes off and don't get any of the new draft stuff, so the mail drops some of the details it is supposed to have.

Test Plan: Disabled prototypes, created a revision, saw summary / test plan in the initial mail.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18771
2017-11-14 12:23:15 -08:00