Summary: Ref T6861. Some discussion in IRC. The behavior of `sort` is somewhat broken when dealing with mixed types. In this particular case, we have both integers and strings.
Test Plan: @epriestley confirmed that this made the ordering of the Celerity map slightly-more-sane.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6861
Differential Revision: https://secure.phabricator.com/D11210
Summary:
Ref T1751. When a commit reverts another commit:
- Add an edge linking them;
- Show the edge in Diffusion.
Next steps are:
- If the reverted commit is associated with a Differential revision, leave a comment;
- Also leave a comment on the commit (no API yet);
- Also trigger an audit by the original commit's author.
Test Plan: Used `scripts/repository/reparse.php --message ...` to parse commits with revert language. Verified they appear correctly in Diffusion, and update Differential.
Reviewers: btrahan, epriestley
Reviewed By: btrahan, epriestley
Subscribers: Korvin, epriestley, cburroughs, joshuaspence, sascha-egerer, aran
Maniphest Tasks: T4896, T1751
Differential Revision: https://secure.phabricator.com/D5846
Summary: Show the full unit test name, including the namespace. Depends on D11208.
Test Plan: Inspected the "Table of Contents" of a diff created //with// D11208 and //without// D11208 applied.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11209
Summary: The default behavior was inadvertedly changed in D11074. This restores the original behavior.
Test Plan: Added a project reviewer to a diff, saw no inverse transaction recorded.
Reviewers: Krenair, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11181
Summary: This class is no longer used after D11125.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11170
Summary: These classes are no longer used after D10649.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11168
Summary: This class is no longer required after D6675.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11169
Summary: This class is no longer used after D6673.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11167
Summary: Ref T5655. Fixes T6849. This is another take on D11131, which was missing the DB migration and was reverted in rP7c4de0f6be77ddaea593e1f41ae27211ec179a55.
Test Plan: Ran `./bin/storage upgrade` and verified that the classes were renamed in the `phabricator_policy.policy` table.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6849, T5655
Differential Revision: https://secure.phabricator.com/D11166
Summary: This class is no longer used after D10965.
Test Plan: `grep`
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11133
Summary: Modernize remaining edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Browsed around and performed various actions include subscribing, unsubscribing and watching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11116
Summary: I was going to fix the variable name as it violates convention, but it is not used anyway.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11139
Summary:
Ref T2783.
This updates PhabricatorRepositoryManagementLookupUsersWorkflow to use ConduitCall to retrieve information about the commit.
Test Plan:
Ran `bin/repository lookup-users rTESTe9683b64d3283f0b2d355fdbf231bc918b5ac0ab --trace` and saw the information returned (by making a request to `diffusion.querycommits` as the omnipotent user, signed with the device key).
Mucked with `cluster.addresses` and saw requests rejected.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: Krenair, epriestley, Korvin
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10403
Summary:
Ref T2783. Ref T6706.
- Add `cluster.addresses`. This is a whitelist of CIDR blocks which define cluster hosts.
- When we recieve a request that has a cluster-based authentication token, require the cluster to be configured and require the remote address to be a cluster member before we accept it.
- This provides a general layer of security for these mechanisms.
- In particular, it means they do not work by default on unconfigured hosts.
- When cluster addresses are configured, and we receive a request //to// an address not on the list, reject it.
- This provides a general layer of security for getting the Ops side of cluster configuration correct.
- If cluster nodes have public IPs and are listening on them, we'll reject requests.
- Basically, this means that any requests which bypass the LB get rejected.
Test Plan:
- With addresses not configured, tried to make requests; rejected for using a cluster auth mechanism.
- With addresses configred wrong, tried to make requests; rejected for sending from (or to) an address outside of the cluster.
- With addresses configured correctly, made valid requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6706, T2783
Differential Revision: https://secure.phabricator.com/D11159
Summary:
Ref T2783. This is basically a more refined version of D10400, which churned a bit on things like SSH key storage, the actual way the signing protocol shook out, etc.
- When Phabricator tries to make an intra-cluster service call as the omnipotent user, sign it with the host's device key.
- Add `bin/almanac register` to say "this host is X device, identified by private key Y". This stores the keypair locally, adds the public key to Almanac, and trusts it.
Net effect is that once a host has been registered, the daemons can make calls to other nodes as the omnipotent user. This is primarily necessary so they can access repository API methods on remote hosts.
Test Plan:
- Ran `bin/almanac register` with various valid and invalid inputs.
- Verified keys get generated/added/stored properly.
- Made a device-signed cluster Conduit call.
- Made a normal old user-signed cluster Conduit call.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11158
Summary: This class is unused after D6679.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11149
Summary: This class is no longer used after D8168.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11152
Summary: This class is no longer required after D10869.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11154
Summary:
These didn't get translated quite right:
- We need to use `$total_count` because some languages have different words for 1, 2-3, and 4+ things (for example). So the strings might translate as:
- alincoln added a reviewer-one ...
- alincoln added reviewers-few ...
- alincoln added reviewers-many ...
- That is, while English has only "reviewer" and "reviewers", other languages have more plural forms, and "reviewer", "reviewers-few" and "reviewers-many" may be completely different words.
- In English, because we know we always have 2+ in this branch and the only special word is for 1, we can just drop this.
- Anyway, the %4$s stuff is counting assuming that $total_count is included in the string, so these were a off by one.
- See also D11160.
There a probably a couple more of these, but they should be easy enough to hunt down as they crop up.
Test Plan: Saw nice strings instead of empty strings, or invalid strings (after D11160).
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11162
Summary: The actual author here usually gets truncated by the extra text, which doesn't seem needed in most (all?) cases.
Test Plan: Look at list of recent commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11127
Summary:
Fixes T6692. Addresses two main issues:
- The write guard would sometimes not get disposed of on exception pathways, generating an unnecessary secondary error which was just a symptom of the original root error.
- This was generally confusing and reduced the quality of reports we received because users would report the symptomatic error sometimes instead of the real error.
- Instead, reflow the handling so that we always dispose of the write guard if we create one.
- If we missed the Controller-level error page generation (normally, a nice page with full CSS, etc), we'd jump straight to Startup-level error page generation (very basic plain text).
- A large class of errors occur too early or too late to be handled by Controller-level pages, but many of these errors are not fundamental, and the plain text page is excessively severe.
- Provide a mid-level simple HTML error page for errors which can't get full CSS, but also aren't so fundamental that we have no recourse but plain text.
Test Plan:
Mid-level errors now produce an intentional-looking error page:
{F259885}
Verified that setup errors still render properly.
@chad, feel free to tweak the exception page -- I just did a rough pass on it. Like the setup error stuff, it doesn't have Celerity, so we can't use `{$colors}` and no other CSS will be loaded.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, chad
Maniphest Tasks: T6692
Differential Revision: https://secure.phabricator.com/D11126
Summary: This reverts commit a896494ce5 which caused T6852
Test Plan: Check, for example, the position of the 'Save Account Settings' button
Reviewers: joshuaspence, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6852
Differential Revision: https://secure.phabricator.com/D11157
Summary:
Removes an unused PhabricatorFeedStory Parameter from all getTitleForFeed() and getApplicationTransactionTitleForFeed() functions.
Ref D11088 Ref T6545
Test Plan: ran all unit tests and viewed some dashboard feeds
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11146
Summary: This class is no longer used after D7370.
Test Plan: `arc lint` and `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11132
Summary: This class is no longer used after D7370.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11129
Summary: This class is no longer required after D10792.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11134
Summary: This class is no longer used after D7585.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11135
Summary: Modernize Dashboard edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Attached a panel to a dashboard, observed the expected comment in the transaction view (both ways).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11114
Summary: Modernize Pholio edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Attached a mock to a task, observed the expected comment in the transaction view (both ways).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11113
Summary: Modernize Project edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Add a member to a project, saw new rows in the `phabricator_project.edge` and `phabricator_user.edge` tables.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11111
Summary: This method has been deprecated for a long time (see D2887 and D6336).
Test Plan:
```lang=bash
> echo '{}' | arc --conduit-uri=http://phabricator.local call-conduit 'maniphest.find'
Waiting for JSON parameters on stdin...
{"error":"ERR-CONDUIT-CALL","errorMessage":"ERR-CONDUIT-CALL: ","response":null}
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11117
Summary: In D10027, the `getName()` method was added to `PhabricatorApplication` to //allow// applications to explicitly declare their names rather than relying on the name being inferred from the name of the `PhabricatorApplication` subclass. I feel that sufficient time has passed such that we can //require// applications to explicitly declare their names.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11119
Summary: I assume this can be removed now?
Test Plan: Not tested, hmm...
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11123
Summary: This class is unused after D6488, maybe we should remove it?
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11124
Summary:
Ref T4245 Make repo callsigns optional
This is far from done and still very ugly. I'm just submitting it to check if i'm solving this in the right places.
Right now there's three places with duplicate code and building the identifierMap in the CommitQuery is very ugly.
If we only want to support this in the user frontend then i could hack it into the Markup rule itself and not touch the CommitQuery. Even uglier but more limited in scope...
Generally this approach will need a lot of "check this first and then try the other" in a few places.
I could move the Repository queries into a specialised PhabricatorRepositoryQuery method (withCallsignOrID) but i'm not sure about that.
Test Plan:
- phid.lookup works with R1 and rTEST (which is the same repo)
- R1 and rTEST euqally work in remarkup (tested in comments).
- Reviewed the following syntax also all works:
rTEST
rTESTd773137a7cb9
rTEST:d773137a7cb9
R1
R1:d773137a7cb9
d773137a7cb9
{rTEST}
{rTESTd773137a7cb9}
{rTEST:d773137a7cb9}
{R1}
{R1:d773137a7cb9}
{d773137a7cb9}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D11050
Summary:
Ref T4411
I'm not quite sure if this is the right place for this as it will be difficult to provide proper user feedback of why we removed a particular subscriber.
Is the ApplicationTransactionEditor generally the right place to extract mentioned phids in comments?
On the other hand in some cases we cannot really give user feedback why a user was not subscribed (e.g.: commits & diffs)
Adding a diff to a repo where the user mentioned has no view permissions the subscriber is currently still added. Still would have to find where this is donet...
Any other places?
Unrelated: Is there any way to remove a subscriber from a commit/audit ?
Test Plan:
- Edited tasks with the mentioned user having view permissions to this specific task and without
- Raised concern with a commit and commented on the audit with the user having view permissions to the repo and without
- Added a commit to a repo with and without the mentioned user having permissions
- Mention a user in a task & commit comment with and without permissions
- Mentioning a user in a diff description & comments with and without permissions to the specific diff
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T4411
Differential Revision: https://secure.phabricator.com/D11049
Summary:
Shows a timeline of all modified settings Fixes T6545
Will show all settings (no pagination, should be not so difficult to add if needed but most installs won't have hundreds of settings changes)
I'm not happy by how the PhabricatorConfigTransaction object is instructed to render the config keys but i don't see any other reasonable way.
We could always show the keys though.
Test Plan: Changed settings and called the history page
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11088
Summary: These classes are named differently from other `PhabricatorEdgeType` subclasses. Rename them for consistency.
Test Plan: I would expect the linter to complain if I missed anything.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11112
Summary: Modernize Differential edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Krenair, epriestley
Differential Revision: https://secure.phabricator.com/D11074
Summary: This is a typo from D11045.
Test Plan: I haven't actually tested this, but the tests from D11045 should apply here.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11108
Summary: These are no longer required after D7076.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11107
Summary: These strings are no longer required after D10678.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11106
Summary: These are no longer required after D11032.
Test Plan: `grep`
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11104
Summary: Modernize Ponder edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: I couldn't actually figure out how to get these strings to show up anywhere.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Krenair, chad, epriestley
Differential Revision: https://secure.phabricator.com/D11083
Summary: Modernize Legalpad edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan:
# Created a Herald rule to require legal signatures on all diffs.
# Created a new diff.
# Saw the transaction string appear correctly.
I wasn't able to check the inverse transaction because there is none. Also, I couldn't see any text on the feed (presumably, transactions authored by Herald do not generate feed items)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Krenair, chad, epriestley
Differential Revision: https://secure.phabricator.com/D11082
Summary:
Ref T2783. This method is kind of goofballs:
- We send a big list of paths to it.
- It sends back a giant blob of HTML.
Instead, just figure out the path we want locally, then fetch the content with `diffusion.filecontentquery`.
Test Plan:
- Viewed main view and directory view, saw a README.
- See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11099
Summary:
Ref T2783. Currently, the repository edit page does some checks agaisnt the local system to look for binaries and files on disk. These checks don't make sense in a cluster environment.
Ideally, we could make a Conduit call to the host (e.g., add something like `diffusion.querysetupstatus`) to do these checks, but since they're pretty basic config things and cluster installs are advanced, it doesn't seem super worthwhile for now.
Test Plan: Saw fewer checks in a cluster repo.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11102
Summary:
Ref T2783. When repository services are defined, skip the check for local repository storage.
In particular, in the Phacility cluster, this path won't exist on web nodes, but nothing will ever try to access it.
Test Plan: Verified new branch gets hit with cluster services defined.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11100
Summary: Fixes T6838. We use a special transaction type for merging, but don't handle it when figuring out mail tags.
Test Plan: Verified merge mail picks up the `maniphest-status` tag.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6838
Differential Revision: https://secure.phabricator.com/D11101
Summary: T5549
Test Plan: Set edit policy on paste, check that only users meeting the policy requirements can edit it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T5549
Differential Revision: https://secure.phabricator.com/D11097
Summary: Ref T5402.
Test Plan:
- Queried archived tasks.
- Grepped for use sites and verified no other callsites are order-sensitive.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11089
Summary: This is a fake hash of many 0s which ends up being a bad link. Detect the fake hash and don't print a link. Fixes T6826.
Test Plan: looked at push log and no longer saw a many 0 entry for the first old ref.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6826
Differential Revision: https://secure.phabricator.com/D11096
Summary: Fixes T6834. Without this asort() call, repositories are ordered by ID, which is arbitrary to the user. This sorts them alphabetically by callsign.
Test Plan: edited a package and saw repos sorted by call sign
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6834
Differential Revision: https://secure.phabricator.com/D11095
Summary: Ref T5402, T6238. Pull this out into a class so the Instances app can embed task views.
Test Plan: Loaded `/daemon/` and examined the content in the tables.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6238, T5402
Differential Revision: https://secure.phabricator.com/D11090
Summary: This is dead code.
Test Plan: These edge types don't actually seem to be used?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11076
Summary: Fix a few minor lint issues.
Test Plan: Ran `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11059
Summary:
Since the default query will sort on this when no query string is
attached we want to make sure the property at least exists.
Otherwise Elasticsearch yells at you: "No mapping found for [dateCreated]
in order to sort on" when you try to search for documents that haven't
been indexed yet.
Test Plan:
Searched for Mocks and Initiatives (no such documents exist in my index)
and got the error. After patching and reinitializing the index, the error
during querying went away.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11056
Summary: I don't have `hg` yet on my new laptop; we should just skip tests if the user is missing binaries. Add a convenience method to do this.
Test Plan: Got clean `arc unit --everything` with no `hg` installed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11051
Summary: Fixes T6795. Fixes T6813. We can give more tailored instructions for surplus schemata than we currently do, and provide more information on resolving them.
Test Plan:
- Ran `storage adjust` with just surplus schemata (friendly warning).
- Ran `storage adjust` with surplus schemata and other serious errors (more severe error).
- Read document.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6795, T6813
Differential Revision: https://secure.phabricator.com/D11054
Summary:
Ref T6817. Ref T5726. These both bypass policy checks, and would allow an attacker who gains control of an administrative account to enable public feed, then view feed stories they could not normally see; or enable feed.http-hooks, then read the posted text.
In the longer term I'd like to remove `feed.public` completely (possibly providing API alternatives, if necessary).
Test Plan: Looked at options in web UI and saw them locked.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6817, T5726
Differential Revision: https://secure.phabricator.com/D11046
Summary: Ref T5402. This more or less "fixes" it but there's probably some polish to do?
Test Plan:
stopped and started daemons. error logs look good.
ran bin/storage upgrade. noted that `adjust` added the appropriate indices for active and archive task.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11044
Summary: Ref T5402. This cleans up some code and sets us up to use this sort of data more easily later.
Test Plan: viewed the daemon console from the web and the log of a specific archived daemon. both looked good. for other callsites looked really, really carefully.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11042
Summary: See rP2fedb6f941d8. We might need a more general version of this since we do some `sudo` stuff elsewhere, but at least on my machine `sudo -n` exits with code 0 when the target user exists but needs a password.
Test Plan:
- Tried to run daemons as root, with no automatic sudo to root. Got a bad result before (phd believed it had executed the daemons) and a good result afterward (phd recognized that sudo failed).
- Tried to run daemons from root, as a non-root user. Got a good result in both cases.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: fabe, epriestley
Differential Revision: https://secure.phabricator.com/D11041
Summary: Fixes T6807. This page shows edit history and has edit options, but is not an edit interface itself per se. Allowing public access is consistent with other interfaces/applications.
Test Plan: Viewed a public project's history page as a public user.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6807
Differential Revision: https://secure.phabricator.com/D11039
Summary:
Fixes T5196
If no phd.user is configured the behaviour is unchanged besides printing a warning when run as root (Usually i would add an exit(1) here but that would break existing installs who do that).
If phd.user is set and the current user is root it will run the daemon as: su USER -c "command" (I'm not sure if this works for every platform needed)
Otherwise it will refuse to start if configured and current user mismatch.
Test Plan: Stopped & Started phd daemon with various users and different phd.user settings including root
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: vinzent, epriestley
Maniphest Tasks: T5196
Differential Revision: https://secure.phabricator.com/D11036
Summary: Show the php.ini setting for max upload or alterantively the in phabricator configured one. Fixes T6663
Test Plan: changed php.ini and alternatively phabricator file upload size settings to minimal values and try to upload a larger file
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, btrahan, chad, epriestley
Maniphest Tasks: T6663
Differential Revision: https://secure.phabricator.com/D11031
Summary:
When the index does not exist and auto_create_index isn't
enabled, running ./bin/index results in a failure. That's
T5990
Instead create an index properly. This also allows us to do
nice things like do a proper mapping and analysis like for
substring matching like outlined by @fabe in T6552.
Test Plan:
Deleted and created index multiple times to verify
proper index creation and usage.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, manybubbles, chasemp, fabe, epriestley
Differential Revision: https://secure.phabricator.com/D10955
Summary:
It's like query_string but fails a little nicer on bad
input. It also allows for limited Lucene syntax; notably
exact string matches with quotation marks.
Fixes T6780
Test Plan:
Tested multiple query constructions, including exact string
matching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T6780
Differential Revision: https://secure.phabricator.com/D11011
Summary: Ref: T6747 add a setup warning if mysql is configured with the default boolean syntax
Test Plan: Run setup checks with default mysql config and then add the desired config option, restart and check again.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T6747
Differential Revision: https://secure.phabricator.com/D11030
Summary: Ref T5833. Ref T6238. These are general capabilities which are particularly useful for synchronizing cluster specifications to instances.
Test Plan:
- Synchronized networks, devices, interfaces, services, bindings and properties to a managed instance.
- Used typeahead.
- Destroyed networks, devices, and services. Saw interfaces and bindings destroyed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238, T5833
Differential Revision: https://secure.phabricator.com/D11024
Summary: Removes the docs sprite in Conpherence with FontAwesome, adds additional icons. Unsure what happens if someone customized this config option.
Test Plan: Added images and files to a Conpherence, saw new icons.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11028
Summary: Fixes an exception on Conpherence.
Test Plan: Use Conpherence. Grep for other locations.
Reviewers: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11027
Summary: Removes unused payments sprite and code, also some unused conpherence generated images. We use images in login (and could use FontAwesome, maybe).
Test Plan: grep codebase, pull up uiexamples icons page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11025
Summary: These were refactored out a while ago
Test Plan: Grep codebase, use Conpherence on desktop, mobile.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11023
Summary:
Ref T4209. Ref T6240. Ref T6238. See D10401 for original discussion.
On OSX, `ssh-keygen` doesn't support PKCS8:
- When we hit an issue with this, raise a more tailored message about it.
- Allow the user to work around the problem with `auth cache-pkcs8 ...`, providing reasonable guidance / warnings.
In practice, this only really matters very much for one key, which I'm just going to make the services extension cache automatically. So it's sort of moot, but good to have around for weird cases and to make testing easier.
Test Plan: Hit error, cached key, got clean asymmetric auth.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4209, T6240, T6238
Differential Revision: https://secure.phabricator.com/D11021
Summary: Fixes T6790. Turn the old method into "new" (old signature) and "newEphemeral". Deploy "newEphemeral" as many places as possible; basically places we are not in the Differential application *and* have no intentions of ever saving the diff. These callsites are also all places we are just trying to get some changesets at the end of the day.
Test Plan: set differential application policy to 'administrators only'. viewed a commit in diffusion and it worked without any errors! i'm just using my thinkin' noodle on the other code paths.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6790
Differential Revision: https://secure.phabricator.com/D11020
Summary: Ref T5833. Ref T6238. This will probably be useful somewhere in the upstream eventually, and is materially useful in the Instances application right now.
Test Plan: tippy typey typey
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238, T5833
Differential Revision: https://secure.phabricator.com/D11016
Summary: database migration + drop old view code. Fixes T5604.
Test Plan: grepped src/ for TYPE_CCS (no hits); viewed some tasks with old cc transactions and noted they still rendered correctly post data conversion
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D11015
Summary: The stacked version of property lists is supposed to display like a normal definition list.
Test Plan: Test the stacked layout in Herald.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11014
Summary:
Fixes T6741. This allows Almanac services to be locked from the CLI. Locked services (and their bindings, interfaces and devices) can not be edited. This serves two similar use cases:
- For normal installs, you can protect cluster configuration from an attacker who compromises an account (or generally harden services which are intended to be difficult to edit).
- For Phacility, we can lock externally-managed instance cluster configuration without having to pull any spooky tricks.
Test Plan:
- Locked and unlocked services.
- Verified locking a service locks connected properties, bindings, binding properties, interfaces, devices, and device properties.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6741
Differential Revision: https://secure.phabricator.com/D11006
Summary:
Ref T2783. When creating a new repository, test for cluster services. If cluster services are available, allocate on a random open service.
Show the service that repositories are allocated on.
Test Plan: Created a new repository, saw it allocate onto an available cluster service.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11003
Summary:
...except the transaction class itself, which still needs some knowledge of these transactions for older installs.
Ref T5245. T5604 and T5245 are now in a similar place -- there's an unknown set of bugs introduced from my changes and there's still old display code lying around with some old transactions in the database. I'll stomp out the bugs if / when they surface and data migration is up next.
This revision also adds a "TransactionPreviewString" method to the edge objects so that we can have a prettier "Bob edited associated projects." preview of this transaction.
Test Plan: added a project from task detail and saw correct preview throughout process with correct project added. bulk removed a project from some tasks. added a project from the edit details pane.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11013
Summary: I find this easier to read
Test Plan: Hover over tooltip area
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11012
Summary: Ref T5245. This is some of the associated cleanup there.
Test Plan:
foreach ManiphestTaskQuery site, I made the change (or not) and tested as follows:
=== Call sites where added needProjectPHIDs ===
- PhabricatorHomeMainController - loaded the home page
- ManiphestBatchEditController - batch edited some tasks (added a project)
- ManiphestConduitAPIMethod - tested implicitly when tested ManiphestUpdateConduitAPIMethod
- ManiphestInfoConduitAPIMethod - used the method via conduit console with input id : 1
- ManiphestQueryConduitAPIMethod - used the method via conduit console with input ids : [1, 2]
- ManiphestUpdateConduitAPIMethod - used the method via conduit with input id : 1 and comment : “asdasds"
- ManiphestReportController - viewed “By User” and “By Project”
- ManiphestSubpriorityController - changed the priority of a task via a drag on manphest home
- ManiphestTaskMailReceiver - updated Task 1 via bin/mail receive-test with a comment that is the README
- ManiphestTaskSearchEngine - loaded Manifest home page
- ManiphestTaskEditController - edited a task
- ManiphestTransactionEditor - closed a blocking task
- ManiphestTransactionSaveController - commented on a task
- PhabricatorProjectProfileController - viewed project with id of 1 that has a few tasks in it
- PhabricatorSearchAttachController - merged tasks together
- DifferentialTransactionEditor - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
- PhabricatorRepositoryCommitMessageParserWorker - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
=== Calls sites where *did not* add needProjectPHIDs (they do not appear in this revision) ===
- PhabricatorManiphestApplication - loaded the home page
- ManiphestGetTaskTransactionsConduitAPIMethod - used the method via conduit console with input ids : [1, 2] ManiphestTaskDetailController - viewed a task with and without associated projects; finished workflow creating a task with a parent
- ManiphestTransactionPreviewController - verified transaction preview showed up properly
- PhabricatorProjectBoardViewController - viewed a board
- PhabricatorProjectMoveController - moved a task around
- ManiphestRemarkupRule - made a task reference like {T123}
- ManiphestTaskQuery - executed a custom query for all tasks with page size of 2 and paginated through some tasks
- ManiphestTaskPHIDType - nothing random seems broken? =D
=== Call sites where had to do something funky ===
- ManiphestHovercardEventListener - loaded hover cards from task mentions
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11004
Summary: Using Project lists on mobile is missing the query menu, this adds it.
Test Plan: test mobile layout, click menu, see query list
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11010
Summary: Makes the header text clickable to the full results.
Test Plan: Click on a few queries.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11008
Summary: Ref T6741. As an optimization, we omit policy checks on ObjectQuery, but need to retain them if the query requests non-view permissions. This primarily affected Almanac properties.
Test Plan: Almanac properties now do full policy checks on ObjectQuery loads.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6741
Differential Revision: https://secure.phabricator.com/D11005
Summary: Ref T2783. In Diffusion -> Edit Repository, we currently have a section called "Local" with options about where the repository is stored. The current name is misleading in a cluster environment, where storage may not actually be local. Shortly, this will also have an option for cluster storage. Call this "Storage" instead.
Test Plan: Edited a repository and poked around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11001
Summary:
Ref T5833. This allows Almanac ServiceTypes to define default properties for a service, which show up in the UI and are more easily editable.
Overall, this makes it much easier to make structured/usable/consistent service records: you can check a checkbox that says "prevent new allocations" instead of needing to know the meaning of a key.
Test Plan: {F251593}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10996
Summary:
Ref T5833. This allows services to be typed, to distinguish between different kinds of services. This makes a few things easier:
- It's easier for clients to select the services they're interested in (see note in T5873 about Phacility). This isn't a full-power solution, but gets is some of the way there.
- It's easier to set appropriate permissions around when modifications to the Phabricator cluster are allowed. These service nodes need to be demarcated as special in some way no matter what (see T6741). This also defines a new policy for users who are permitted to create services.
- It's easier to browse/review/understand services.
- Future diffs will allow ServiceTypes to specify more service structure (for example, default properties) to make it easier to configure services correctly. Instead of a free-for-all, you'll get a useful list of things that consumers of the service expect to read.
The "custom" service type allows unstructured/freeform services to be created.
Test Plan:
- Created a new service (and hit error cases).
- Edited an existing service.
- Saw service types on list and detail views.
- Poked around new permission stuff.
- Ran `almanac.queryservices` with service class specification.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10995
Summary:
Ref T6742. Root cause of the issue:
- Daemon was running on a machine with a very long host name, which produced a lease name which was longer than 64 characters.
- MySQL wasn't set in STRICT_ALL_TABLES.
- The daemon would `UPDATE .. SET leaseOwner = <very long string>` to lock a task, and MySQL would silently truncate.
- The daemon would then try to select the locked task, but fail, because there's no matching lease owner.
To resolve this, use only the first 32 characters of the hostname. See IRC for more discussion.
Test Plan: Will confirm with reporter.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6742
Differential Revision: https://secure.phabricator.com/D10998
Summary: We didn't load enough data for this case for the custom view class Pholio uses. Fixes T6770. Re-jiggers the signature of the loadImages function in the PholioMockQuery to get there so as to not duplicate any business logic.
Test Plan: made a pholio mock with lots of inline comments. pre-patch "show older" fatals and post-patch "show older" works
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6770
Differential Revision: https://secure.phabricator.com/D11002
Summary: If no Diviner groups are defined that this line currently throws a fatal exception... make this a little more safe.
Test Plan: Applied to our install, no more exceptions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10999
Summary:
Ref T5955. Ref T2783.
- Removes the "temporary" type. I was going to use this for T3628 but it started taking more time than I wanted to spend on it.
- Add a "cluster" type, which is an internal-only token type used within a cluster. This token value is never shown to the user.
- Automatically generate, use, and cycle cluster tokens.
Test Plan:
- Diffusion (mostly) works with a repository configured to use a remote service.
- Saw cluster tokens generate; terminated a cluster token and saw it regenerate.
- Viewed cluster token in settings panel and saw nice explanatory text instead, as expected (we might just hide these eventually).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783, T5955
Differential Revision: https://secure.phabricator.com/D10990
Summary:
Ref T5955.
- Add `conduit.getcapabilities` to help arc (and other clients) determine formats, protocols, etc., the server supports.
- Fixes T3117. Add a more modern version of the handshake workflow that allows all generated tokens to remain valid for an hour.
- Generally, add a CLI token type. This token type expires after an hour when generated, then becomes permanent if used.
Test Plan:
- See D10988.
- Ran `conduit.getcapabilities` and inspected output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3117, T5955
Differential Revision: https://secure.phabricator.com/D10989
Summary:
- Ref T5955. Accept the tokens introduced in D10985 as an authentication token.
- Ref T3628. Permit simple `curl`-compatible decoding of parameters.
Test Plan:
- Ran some sensible `curl` API commands:
```
epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/user.whoami?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk" ; echo
{"result":{"phid":"PHID-USER-cvfydnwadpdj7vdon36z","userName":"admin","realName":"asdf","image":"http:\/\/local.phacility.com\/res\/1410737307T\/phabricator\/3eb28cd9\/rsrc\/image\/avatar.png","uri":"http:\/\/local.phacility.com\/p\/admin\/","roles":["admin","verified","approved","activated"]},"error_code":null,"error_info":null}
```
```
epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/differential.query?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk&ids[]=1" ; echo
{"result":[{"id":"1","phid":"PHID-DREV-v3a67ixww3ccg5lqbxee","title":"zxcb","uri":"http:\/\/local.phacility.com\/D1","dateCreated":"1418405590","dateModified":"1418405590","authorPHID":"PHID-USER-cvfydnwadpdj7vdon36z","status":"0","statusName":"Needs Review","branch":null,"summary":"","testPlan":"zxcb","lineCount":"6","activeDiffPHID":"PHID-DIFF-pzbtc5rw6pe5j2kxtlr2","diffs":["1"],"commits":[],"reviewers":[],"ccs":[],"hashes":[],"auxiliary":{"phabricator:projects":[],"phabricator:depends-on":[],"organization.sqlmigration":null},"arcanistProjectPHID":null,"repositoryPHID":null,"sourcePath":null}],"error_code":null,"error_info":null}
```
- Ran older-style commands like `arc list` against the local install.
- Ran commands via web console.
- Added and ran a unit test to make sure nothing is using forbidden parameter names.
- Terminated a token and verified it no longer works.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3628, T5955
Differential Revision: https://secure.phabricator.com/D10986
Summary:
Ref T5955. Summary of intended changes:
**Improve Granularity of Authorization**: Currently, users have one Conduit Certificate. This isn't very flexible, and means that you can't ever generate an API token with limited permissions or IP block controls (see T6706). This moves toward a world where you can generate multiple tokens, revoke them individually, and assign disparate privileges to them.
**Standardize Token Management**: This moves Conduit to work the same way that sessions, OAuth authorizations, and temporary tokens already work, instead of being this crazy bizarre mess.
**Make Authentication Faster**: Authentication currently requires a handshake (conduit.connect) to establish a session, like the web UI. This is unnecessary from a security point of view and puts an extra round trip in front of all Conduit activity. Essentially no other API anywhere works like this.
**Make Authentication Simpler**: The handshake is complex, and involves deriving hashes. The session is also complex, and creates issues like T4377. Handshake and session management require different inputs.
**Make Token Management Simpler**: The certificate is this huge long thing right now, which is not necessary from a security perspective. There are separate Arcanist handshake tokens, but they have a different set of issues. We can move forward to a token management world where neither of these problems exist.
**Lower Protocol Barrier**: The simplest possible API client is very complex right now. It should be `curl`. Simplifying authentication is a necessary step toward this.
**Unblock T2783**: T2783 is blocked on nodes in the cluster making authenticated API calls to other nodes. This provides a simpler way forward than the handshake mess (or enormous-hack-mess) which would currently be required.
Test Plan:
- Generated tokens.
- Generated tokens for a bot account.
- Terminated tokens (and for a bot account).
- Terminated all tokens (and for a bot account).
- Ran GC and saw it reap all the expired tokens.
NOTE: These tokens can not actually be used to authenticate yet!
{F249658}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D10985
Summary:
Two goals:
- If an attacker compromises an administrator account (without compromising the host itself), they can currently take advantage of vulnerabilities in prototype applications by enabling the applications, then exploiting the vulnerability. Locking this option requires CLI access to enable prototypes, so installs which do not have prototypes enabled have no exposure to security issues in prototype applications.
- Making this very slightly harder to enable is probably a good thing, given the state of the world and support.
Test Plan: Verified that web UI shows the value is locked and instructs the user to update via the CLI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10993
Summary: Via HackerOne. An attacker with access to both Phame and the filesystem could potentially load a skin that lives outside of the configured skin directories, because we had insufficient checks on the actual skin at load time.
Test Plan: Attempted to build a blog with an invalid skin; got an exception instead of a mis-load of a sketchy skin.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10992
Summary: Fixes T6748. This just didn't get aligned when CCs became a modern transaction.
Test Plan: Added a CC to a task, used `bin/mail show-outbound` to verify it showed up as a CC tag.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6748
Differential Revision: https://secure.phabricator.com/D10991
Summary:
Ref T2783. This is primarily exploratory and just figuring out what we're blocked on:
- Allow a Repository to be bound to a Service. The Service may eventually define multiple read/write nodes, etc.
- There's no UI to do this binding yet, you have to touch the database manually.
- If a repository is bound to a Service, effect Conduit calls via calls to the remote service instead of executing them in-process.
- These don't actually work yet since there's no authentication (see T5955).
Test Plan:
- Made a nice Service with a nice Binding to a nice Interface on a nice Device.
- Force-associated a repository with the service using a raw MySQL query.
- Saw Phabricator try to make a remote call to the service (on localhost) and fail because of missing auth stuff.
- Also ran `almanac.queryservices`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10982
Summary: Fixes T6595. This diff has two issues as is... 1) the differential data fetching is pretty cheesey, but it looks like we can't just issue three separate databases to get the right data? 2) the translations break, since I am turning this into a string (and not an int) so the whole pluralization bit fails. I think 1 is okay as is and 2 needs to be fixed though I am not sure how to best do that...
Test Plan: loaded home page and it looked nice...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6595
Differential Revision: https://secure.phabricator.com/D10979
Summary:
Fixes T6651, T6682. Since policy is defined by ancestry, you can't make things outside the core tree.
An alternative fix would be to automagically stub everything in these cases. This has potential negative policy implications - consider making a public document with several levels of depth that automagically stubs out its ancestry as public - and additionally the PhabricatorApplicationTransactionEditor framework would make this very tricky code (i.e. you are expected to validateTransactions in said hook *and* return an error if things aren't valid and not do some automagic stubbing, etc.)
Test Plan: tried to move a doc from location/that/exists to locationz/thatz/dontz/existz/ and got an error message with links to each missing doc. tried to create a doc at locatonz/thatz/dontz/existsz/ and got an error message with links to each missing doc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6682, T6651
Differential Revision: https://secure.phabricator.com/D10978
Summary: Fixes T6734. This is a very generic fix, which basically attaches the subscribers if necessary. This seems like a good idea given there's some crazy generic code doing this sort of thing? This would end up being a new pattern for these types of objects that may be loaded by a general object query but then get some editor action against them.
Test Plan: I can't actually reproduce this in my sandbox so I'll verify live again.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6734
Differential Revision: https://secure.phabricator.com/D10976
Summary:
There's a comma to the lower-left of my profile picture here:
{F248962}
This is on a page like https://secure.phabricator.com/F248948
What's happening is that some `render()` method is returning a valid result like `array($stuff, null)`. This is getting passed to JS as an array, which is implicitly `join()`'ing it into a string, adding a comma.
Instead, make sure we render these to strings on the server side before shipping them to the client.
Test Plan: No more comma on file previews.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10974
Summary: Ref T5604. Found this trying to open T5604 live. Basically this internal query needs the needSubscriberPHID set to true.
Test Plan: doing it live
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D10975
Summary: Fixes T6731. I don't really understand the intent behind the two view classes here, but to get this to work I need to pass yet more data to the lower-level class.
Test Plan: Viewed a task with many comments. Clicked "show older". Quoted everything I could. Verified for each quote that it quoted correctly, inlcuding linking to the prior transaction.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6731
Differential Revision: https://secure.phabricator.com/D10973
Summary: (Needed a clean branch). Moves the field up and renames to Query
Test Plan: Visit Maniphest Search, see new field, test a query
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10971
Summary: Fixes T6727. Repro is: mention a task on another task, in a comment.
The inverse edge editor applying the "alincoln mentioned this in <other task>" transaction doesn't have enough data to execute Herald rules.
Just don't try to execute the rules, since they don't make much sesne from a product perspective and are tricky from a technical perspective.
Test Plan: Commented on `T1` with `T2` in comment body and a Herald rule that examines subscribers.
Reviewers: btrahan
NOTE: Cowboy committing this since any task mention fatals.
Summary: Fixes T5604. This should fix some random bugs, lets us move forward more easily, and all that good stuff about killing code debt.
Test Plan:
- Conduit method maniphest.createtask
- verified creating user subscribed
- verified subscription transaction
- Conduit method maniphest.update
- verified subscribers set as specified to ccPHIDs parameter
- verified subscription transaction
- Herald
- verified herald rule to add subscriber worked
- verified no subscribers removed accidentally
- edit controller
- test create and verify author gets added IFF they put themselves in subscribers control box
- test update gets set to exactly what user enters
- lipsum generator'd tasks work
- bulk add subscribers works
- bulk remove subscriber works
- detail controller
- added myself by leaving a comment
- added another user via explicit action
- added another user via implicit mention
- task merge via search attach controller
- mail reply handler
- add subscriber via ./bin/mail receive-test
- unsubscribe via ./bin/mail receive-test
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D10965
Summary: I think this is what you're after?
Test Plan: clicky clicky
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10966
Summary: Fixes T6598, "Primary Hashtag" field should only be visible in edit mode of existing projects.
Test Plan: Create project, "Primary Hashtag" field should be hidden. Edit an existing project, "Primary Hashtag" field should appear above "Additional Hashtags" as before.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6598
Differential Revision: https://secure.phabricator.com/D10964
Summary:
Ref T2783. ConduitCall currently has logic to pick a random remote server, but this is ultimately not appropriate: we always want to send requests to a specific server. For example, we want to send repository requests to a server which has that repository locally. The repository tier is not homogenous, so we can't do this below the call level.
Make ConduitCall always-local; logic above it will select ConduitCall for an in-process request or do service selection for an off-host request via ConduitClient.
Test Plan:
- Browsed some pages using ConduitCall, everything worked.
- Grepped for removed stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10959
Summary: This adds back the title to the header link and scans through the codebase for instances where
Test Plan: Tested as many ObjectItemLists as I could find (each app homepage), there may be outliers, but can resolve those individually.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10961
Summary: Ref T6723. This allows you to hover over truncated headers and get the full text if needed.
Test Plan:
Hover over header, see full title
{F248197}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6723
Differential Revision: https://secure.phabricator.com/D10958
Summary: Fixes T6562, Title/Description querying for Passphrase
Test Plan: Open Passphrase, open advanced queries, enter a title and/or description. Search results should show credentials matching the search.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6562
Differential Revision: https://secure.phabricator.com/D10953
Summary: Ref T6713. We were dropping latest transaction ID. This should fix the "easy" part of T6713.
Test Plan: tried to add participants and it worked! (removing participants only allows yourself, but that worked too.)
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6713
Differential Revision: https://secure.phabricator.com/D10952
Summary: Fixes T6719. At some point, we added automagical subscriptions via @mentions, and these were failing in project descriptions from a lack of an implementation in the editor. Said "implementation" is to do nothing, but it needs to be there nonetheless.
Test Plan: updated a project mentioning someone in the description and it worked. also saw a 'subscriber added' transaction
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6719
Differential Revision: https://secure.phabricator.com/D10951
Summary: Cleans up spacing, updates to fonts instead of images. Fixed some mobile issues.
Test Plan:
Test with and without counts on desktop, tablet, mobile. Test layout in FF, Chrome, IE.
{F246745}
{F246746}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10948
Summary: Fixes T6664, clicking search icon in empty search field should link to advanced search
Test Plan: navigate to home page, click search icon or click into search box and hit enter. Advanced search page should open.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6664
Differential Revision: https://secure.phabricator.com/D10947
Summary:
The `$timeline` variable is undefined. I was seeing the following error in the logs:
```
EXCEPTION: (RuntimeException) Undefined variable: timeline at [<phutil>/src/error/PhutilErrorHandler.php:210]
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/harbormaster/controller/HarbormasterStepEditController.php:205]
#1 HarbormasterStepEditController::processRequest() called at [<phabricator>/src/aphront/AphrontController.php:33]
#2 AphrontController::handleRequest(AphrontRequest) called at [<phabricator>/webroot/index.php:103]
```
Test Plan: Created a build step without a fatal error.
Reviewers: btrahan, hach-que, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10941
Summary:
Fixes T6702. Ref T3554. Currently, tasks can be cancelled, retried and freed from the web UI by any logged in user.
This isn't appreciably dangerous (I can't come up with a way that a user could do anything security-affecting), but I think I probably intended this to be admin-only, but these actions should move to the CLI anyway.
Move them to the CLI. Lay some groundwork for some future `bin/worker cancel --class SomeTaskClass`, but don't implement that yet.
Test Plan: Used `cancel`, `retry` and `free` from the CLI. Hit all the error/success states.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3554, T6702
Differential Revision: https://secure.phabricator.com/D10939
Summary: Ref T3669. Probably. Adds a yellow warning at the top of the Diff View and makes the comment draft icon yellow on lists of revisions.
Test Plan:
Test a diff with many warnings, see warning. Test a diff with draft comments, see warning. Test new icon in list view.
{F230133}
{F230134}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3669
Differential Revision: https://secure.phabricator.com/D10789
Summary: Fixes T6699. We need to "loadInlineComments" consistently, though for an unexpected reason - this mutates the $changesets to include all $changesets that have an associated inline comment, which is necessary to make them render properly.
Test Plan: Took a diff with inline comments and updated it, noting the inline comments disappeared. applied this patch and the inlines reappeared.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6699
Differential Revision: https://secure.phabricator.com/D10935
Summary: Change icon for Settings app to more match previous. Also align plus icon a little better.
Test Plan: Lots of staring.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10934
Summary: Fixes T6694. Ref T4712. Turns out the logic here was slightly incorrect; we don't want to use the id of the last thing we hid but rather the first thing we show. I had garbage test data ("asdsadsadsa", etc) I guess so I didn't notice this.
Test Plan: made a new task where user a and user b alternated 3 comments each, cooperatively numbering them from 1 - 20. as both users, showed older transactions. pre-patch the issue described in T6694 occurred and post patch I saw the entire counting sequence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712, T6694
Differential Revision: https://secure.phabricator.com/D10933
Summary: we still need to be pager-sensitive, but otherwise this "show all" stuff is dead, dead dead...! Ref T4712. I think we can close the book on T4712 with one more diff to clean up the array_reverse / reverse paging stuff? That diff is probably a bit tricky as it involes auditing every TransactionQuery callsite...
Test Plan: viewed a task with a lot of transactions. clicked "show older" and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10926
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: Updates header to use font-icons instead of images.
Test Plan: Test desktop and mobile layouts, Chrome, FF, Safari, IE.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10930
Summary: Only necessary for edits, only bother if the comment version is greater than 1. Ref T6690. This is another way to fix T6690 -- this check will never run since you can't edit a conpherence comment -- **but** the fix already applied should happen too to future proof Conpherence.
Test Plan: made a comment on a diff - success. edited the comment and mentions were generated.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10928
Summary: Fixes T6690. The editor innards end up loading the conpherence object, whose policy is dictated by these participation objects.
Test Plan: pre patch could not create new conpherences. post patch I can create conpherences! i can also add people to conpherences and it works.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10927
Summary:
Ref T4712. Specifically...
- Differential
- needed getApplicationTransactionViewObject() implemented
- Audit
- needed getApplicationTransactionViewObject() implemented
- Repository
- one object needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true)
- Ponder
- BONUS BUG FIX - leaving a comment on an answer had a bad redirect URI
- both PonderQuestion and PonderAnswer needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on both "history" controllers
- left a "TODO" on buildAnswers on the question view controller, which is non-standard and should be re-written eventually
- Phortune
- BONUS BUG FIX - fix new user "createNewAccount" code to not fatal
- PhortuneAccount, PhortuneMerchant, and PhortuneCart needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on Account view, merchant view, and cart view controller
- Fund
- Legalpad
- Nuance
- NuanceSource needed PhabricatorApplicationTransactionInterface implemented
- Releeph (this product is kind of a mess...)
- HACKQUEST - had to manually create an arcanist project to even be able to make a "product" and get started...!
- BONUS BUG FIX - make sure to "setName" on product edit
- ReleephProject (should be ReleepProduct...?), ReleephBranch, and ReleepRequest needed PhabricatorApplicationTransactionInterface implemented
- Harbormaster
- HarbormasterBuildable, HarbormasterBuild, HarbormasterBuildPlan, and HarbormasterBuildStep all needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) all over the place
Test Plan: foreach application, viewed the timeline(s) and made sure they still rendered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10925
Summary: Ref T4712. These are a bit time consuming to test so might as well send off a batch now and again.
Test Plan: foreach impacted controller, made sure the timeline rendered as it did before. for project column and config, noted the "should terminate" UI was also rendered unlike before.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10923
Summary: Fixes T6648. We do some automagical hotness based on the text you enter in remarkup textareas - e.g. adding projects or mentioning other objects. Refine the code here so that even when just editing a comment we build these transactions and apply them.
Test Plan: edited a comment and noted new mentions and projects showed up appropriately...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6648
Differential Revision: https://secure.phabricator.com/D10922
Summary:
Ref T4712. Thus far, it seems that most "non-standard" things can be done pretty easily in the controller. Aside from deploying, this diff had to fix a few bugs / missing implementations of stuff.
(Notably, PhabricatorAuthProviderConfig, HeraldRule, PhabricatorSlowvotePoll, and AlmanacNetwork needed to implement PhabricatorApplicationTransactionInterface, PhabricatorAuthAuthProviderPHIDType had to be added, and a rendering bug in transactions of type PhabricatorOAuth2AuthProvider had to be fixed.)
Test Plan: Almanac - looked at binding, device, network, and service view controllers and verified timeline displayed properly. Herald - looked at a rule and verified timeline. Slowvote - looked at a vote and verified timeline. Auth - looked at an auth provider (Facebook) and verified proper display of transactions within timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10921
Summary: These have all been modernized.
Test Plan: Browse Diffusion on a narrow screen.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10920
Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.
Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10887
Summary: Converts PHUIObjectItemView to use display: table rows and columns for more flexible layouts. Slightly increases spacing, improves mobile layouts. Fixes T5502
Test Plan: Tested in multiple applications and UIExamples. Ran through mobile, tablet, and desktop break points. Used IE8-IE10, Firefox, Chrome, Safari on both Mac and Windows.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5502
Differential Revision: https://secure.phabricator.com/D10917
Summary:
Fixes T6619. In `{Xnnn key=value, key=value}` we did not require a separator between the object and the key-value part. This could lead to `{rX11aaa}` being parsed as `{rX11 aaa}`, i.e. a reference to `rX11` with parameter `aaa` set.
Instead, require a space or comma before we'll parse key-value parts of embedded objects.
Test Plan:
Added and executed unit tests.
{F242002}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6619
Differential Revision: https://secure.phabricator.com/D10915
Summary: I think this was a "hacked" sub thing that never got updated when we switched to a real editor? I am not 100% sure how these methods are used, so please let me know if I should expand my test plan. Fixes T6659.
Test Plan: made a diff from the web ui, looked up the phid from mysql, ran bin/remove destroy <phid>, and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6659
Differential Revision: https://secure.phabricator.com/D10911
Summary: Fixes T6658.
Test Plan: made a diff with no repository and default policy and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6658
Differential Revision: https://secure.phabricator.com/D10910
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Fixes positioning issues by creating another container to hold the abs. positioned arrows. (Issues primarily presented on Workboards).
Test Plan: Test UI arrows on a workboard, applciation launcher, and in UIExamples.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10897
Summary:
Ref T6615. Mixing ASC and DESC ordering on a multipart key makes it dramatically less effective (or perhaps totally ineffective).
Reverse the meaning of the `priority` column so it goes in the same direction as the `id` column (both ascending, lower values execute sooner).
Test Plan:
- Queued 1.2M tasks with `bin/worker flood`.
- Processed ~1 task/second with `bin/phd debug taskmaster` before patch.
- Applied patch, took ~5 seconds for ~1.2M rows.
- Processed ~100-200 tasks/second with `bin/phd debug taskmaster` after patch.
- "Next in Queue" query on daemon page dropped from 1.5s to <1ms.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aklapper, 20after4, epriestley
Maniphest Tasks: T6615
Differential Revision: https://secure.phabricator.com/D10895
Summary: Ref T6615. Ref T3554. We need better tooling around the queue eventually, so start here.
Test Plan: Added 100K+ tasks locally with `bin/worker flood`. Executed some of them with `bin/phd debug taskmaster` (we already have a TestWorker, used in unit tests).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3554, T6615
Differential Revision: https://secure.phabricator.com/D10894
Summary: Helpful shortcut, I make quite a few at least
Test Plan: Click on dropdown, then Project. Get to Project Create page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10893
Summary: Fixes T6567, lets admins set a default policy for new Projects.
Test Plan: Changed the default Policy, created a new Project. Project had correct defaults.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6567
Differential Revision: https://secure.phabricator.com/D10890
Summary: **WIP** This adds default capability to the Files application
Test Plan: Set default to public, go to Files page, see public preset. Upload File. Doesn't work.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6564
Differential Revision: https://secure.phabricator.com/D10888
Summary:
Ref T6240. Some discussion in that task. In instance/cluster environments, daemons need to make Conduit calls that bypass policy checks.
We can't just let anyone add SSH keys with this capability to the web directly, because then an adminstrator could just add a key they own and start signing requests with it, bypassing policy checks.
Add a `bin/almanac trust-key --id <x>` workflow for trusting keys. Only trusted keys can sign requests.
Test Plan:
- Generated a user key.
- Generated a device key.
- Trusted a device key.
- Untrusted a device key.
- Hit the various errors on trust/untrust.
- Tried to edit a trusted key.
{F236010}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6240
Differential Revision: https://secure.phabricator.com/D10878
Summary: Fixes T4652, adding workboard link to emails
Test Plan: Move a task in a workboard from one column to another. Email notification should contain "WORKBOARD" section with link to that workboard
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4652
Differential Revision: https://secure.phabricator.com/D10889
Summary: For actions like "Close" that are in theory stopping the timeline, we should display some disruption to the line itself.
Test Plan:
Tested in UIExamples
{F236077}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10884
Summary: Ref T1217, Add link to email preferences to email template
Test Plan: Add comment to object like Maniphest task, check that email has a footer with a link to email preferences.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T1217
Differential Revision: https://secure.phabricator.com/D10883
Summary: Uses the check icon for closed, which is the primary action taken.
Test Plan:
Close as a duplicate, seen new icon.
{F236048}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10882
Summary: Finishes up work on PHUIInsetForm, cleans up the UI a bit when editing multiple tasks
Test Plan:
Edit multiple tasks, apply some new projects, still works as expected.
{F236044}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10881
Summary: Ref T5833. We try to load bindings even if we get no services back.
Test Plan: Queried invalid results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10880
Summary: Fixes T3189. Now if you say #projects in a commit message they will associate nicely with the commit. Also we record transactions about all this project editing fun.
Test Plan: tested migration by associating some projects with commits and verifying they still showed up post migration. tested adding / removing projects by doing so from the UI, noting transactions written nicely as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Projects: #projects
Maniphest Tasks: T3189
Differential Revision: https://secure.phabricator.com/D10877
Summary:
When a logged-out user views the home page, we currently compute a meaningless "assigned tasks" count which just counts every open task. Don't do this.
Ideally `ManiphestTaskQuery` should get some tightening up too (that is, `withOwners(array(null))` should not select every task), but that might affect other stuff and the performance implications of counting every open task are affecting WMF, so just fix the immediate issue for now.
Test Plan:
- Viewed homepage as a logged out user, no assigned task count.
- Viewed homepage as a logged-in user, still saw assigned task count.
Reviewers: btrahan, chad, chasemp
Reviewed By: chasemp
Subscribers: 20after4, epriestley
Differential Revision: https://secure.phabricator.com/D10876
Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.
Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6152
Differential Revision: https://secure.phabricator.com/D10875
Summary: Fixes T6200. Ref T6237. When creating a diff from the web view, allow the user to select the repository at that time. When viewing a diff that has no associated revision and then creating a revision, pass along the repository phid to the create revision controller. Within the create revision controller, default the repository selector to this repository phid. Finally, in the editor, stop aggressively resetting the repository phid for every TYPE_UPDATE; rather, do so if its not a new object -- the diff should reign supreme in that case -- or if there's no repository -- let the diff be the guide.
Test Plan:
- made a diff with an associated repo, made a revision from the diff, saw the associated repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo to nothing and it stuck on save!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6200
Differential Revision: https://secure.phabricator.com/D10872
Summary: Ref T6237. This sets us up for some future work like T6152, T6200 and generally cleaning up this workflow a bit. Tried to do as little as possible so not exposing transaction view yet. (Though that timeline is going to be a little funky in the common case of just the lone create transaction.)
Test Plan: made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237
Differential Revision: https://secure.phabricator.com/D10869
Summary: Ref T6343, adding HTMLMailMode to remarkup, and most objects should now be processed and appear pretty in emails.
Test Plan: Add a comment to a Maniphest task containing a mention of an object like '{T1}' or 'T1'. Emails should show a styled version of the object similar to how the object looks in the context of the Maniphest task in the UI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6343, T2617
Differential Revision: https://secure.phabricator.com/D10859
Summary: The shield is just confusing. In one case it doesn't work, and in the other case it just shows you a copy of the file you can see just below except in red. Fixes T4599, T1211. Note T1211 proposed not showing the "move away" file **at all** but I think removing the shield fixes the source of confusion. The code here is a bit if / else if / else if... heavy but this is logically sound.
Test Plan: made a diff where i moved a file then edited it in the new location. viewed diff, saw confusing shield, dropped caches, applied patch, viewed diff and saw no shield. made a diff where I moved a file and didn't edit in new location and saw similar shield disappearness.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1211, T4599
Differential Revision: https://secure.phabricator.com/D10865
Summary: Ref T6576. This avoids generating almost-empty HTML mail bodies for mail which incorrectly has no HTML body.
Test Plan: Generated some mail locally; the specific hook case is a pain for me to hit right now. Will push and dig in if that doesn't fix it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6576
Differential Revision: https://secure.phabricator.com/D10863
Summary:
Ref T4209. Depends on D10402.
This updates Conduit to support authenticating calls from other servers by signing the request parameters with the sending server's private key and verifying it with the public key stored in the database.
Test Plan:
- Made like 500 bad calls using the stuff in D10402.
- Made a few valid calls using the stuff in D10402.
Reviewers: hach-que, btrahan, #blessed_reviewers
Reviewed By: btrahan, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T6240, T4209
Differential Revision: https://secure.phabricator.com/D10401
Summary:
Ref T5833. Just building one query for now which returns the whole binding + interface + network + device tree. Maybe this will get split up in the future.
This will allow web hosts to call the central Almanac and pull instance configuration, authenticating with SSH.
Test Plan: {F234443}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10862
Summary: Fixes T6525, adds cc and tos to html emails
Test Plan: send html and plain emails, see new stuff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6525
Differential Revision: https://secure.phabricator.com/D10857
Summary: Fixes T6243.
Test Plan: simulated having this on by adding 'ONLY_FULL_GROUP_BY' to the $modes array. verified the error message looked nice {F233954}.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6243
Differential Revision: https://secure.phabricator.com/D10856
Summary: Fixes T4946. Theoretically.
Test Plan:
iiam
also unit tests.
also
```
cd /var/repo/X
git remote remove origin # simulates origin-missing clone under 1.7.1
cd /path/to/phabricator
./bin/repository pull X
```
and observed no errors
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4946, T5938
Differential Revision: https://secure.phabricator.com/D10855
Summary: This upgrades 1up view from "does not work" back to "barely works".
Test Plan: view diff, 1up and 2up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10854
Summary: Fixes T3046. Git has no minimum or bad versions; svn has no minimum and the bad version of 1.7.1, and mercurial has a minimum of 1.9 and bad versions 2.2 and 2.1. These error messages are specific about the problem but do not include the details as to what specifically is bad about a given version. I don't think that's really necessary - the details don't help solve the problem and its probably booooooring at best to the average user. The details about a bad version are included in the code however.
Test Plan: hardcoded a VCS array to let me test all the VCS stuff. added some phlog() calls to make sure the VCS version parsing stuff was working correctly relative to the version(s) on my system. played around with setting minimum versions and bad versions and saw good results depending on what I set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3046
Differential Revision: https://secure.phabricator.com/D10852
Summary:
Ref T6238. I'm building the instance management application now, but not putting it in the upstream -- I think the only use case for it is to build SAAS. If someone comes up with a use case (maybe a college course that wants to create an instance per-class or something?) we could open it up eventually, but it seems cleaner to keep it out of the upstream until we have such a use case.
I need to add schema patches. Make it easier for a subclass to just "add all the patches in this directory", like "autopatches/" works.
Test Plan:
- Ran `bin/storage status`, saw all normal patches still valid.
- In some future diff, the instances application will use this to apply patches.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238
Differential Revision: https://secure.phabricator.com/D10848
Summary: Fixes T6472. I was curious to see how many reports we'd get but we didn't get any I know of... Still, this is a nice convenient link.
Test Plan:
edited a wiki doc title + body. ran /bin/mail to see the body and liked what I saw
```TEXT BODY
btrahan changed the title from "new title is a good thing" to "new title is a good thing asdsadsadsadsada".
btrahan edited the document content.
DOCUMENT DIFF
http://phalanx.dev/phriction/diff/36/?l=3&r=4
DOCUMENT DETAIL
http://phalanx.dev/w/```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6472
Differential Revision: https://secure.phabricator.com/D10850
Summary: Fixes T1768. This is mostly a data cleanliness issue as duplicate rows don't really do anything, but let's clear it up now.
Test Plan: made some duplicate rows by adding the same auditor multiple times. ran ./bin/storage upgrade and it worked perfectly!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1768
Differential Revision: https://secure.phabricator.com/D10849
Summary: ...way way down in PhabricatorController. Use it on ManiphestTaskDetailController to test it. Ref T4712. I think the pager logic to be added as part of T4712 can safely reside entirely within this method. As I said earlier, 5 parameters is a lot, so I don't really want to add more. Next diff would do the pagination logic and the diff after that would deploy it everywhere. If while deploying it everywhere I find something off, that will be a different diff.
Test Plan: viewed maniphest tasks and they looked as spiffy as ever.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10844
Summary:
Fixes T6543. This was slightly trickier than I thought.
The actual inputs to this are: author, total affected count, added count, added list, removed count, removed list.
We weren't accounting for "total affected count" (used to select the correct word for "reviewers", e.g. "reviewers-few" vs "reviewers-many").
Test Plan: {F233357}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6543
Differential Revision: https://secure.phabricator.com/D10846
Summary: Fixes T5015, Allow Herald rules for Maniphest to act on task status changes.
Test Plan: Create Herald rule for Maniphest tasks to flag a task with status "wontfix". Change status of Maniphest task to "wontfix". Task should be flagged.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5015
Differential Revision: https://secure.phabricator.com/D10842
Summary: Fixes T6175. This particular option is only editable via the Applications application so let the user know that.
Test Plan: saw error message and clicked through to applications app. tried another locked option and saw old message
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6175
Differential Revision: https://secure.phabricator.com/D10841
Summary: ...if pertinent environment variables are set that is... Fixes T4151. This is the last piece in making repository creation somewhat easier.
Test Plan: made a new repo and noted that http serving was on r/w and ssh serving was still off, as expected for my environment configuration
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4151
Differential Revision: https://secure.phabricator.com/D10839
Summary:
Fixes T5492. I figured this would be easier to just fix than write a guide for; it actually took me an hour, but I spent like 75% of that futzing with my editor.
- The Move controller currently accepts either a slug or an ID. I can't find any callsites which pass a slug, and this doesn't make sense. Pretty sure this was copy/pasted from Edit or something. Only accept IDs.
- Slightly modernize the Move controller (newDialog(), handleRequest(), $viewer).
- When the user enters a bad slug, warn them that we're going to fix it for them and let them accept or reject the changes.
- Don't prefill the edit note (this feels inconsistent/unusual).
- On the form, label the input "Path" instead of "URI".
- Show the old path, to help remind the user what the input should look like.
- When a user tries to do a no-op move, show a more tailored message.
- When the user tries to do an overwriting move, explain how they can fix it.
- When normalizing a slug like `/question/???/mark/`, make it normalize to `/question/_/mark`.
Test Plan:
- Tried to move a document to itself.
- Tried to overwrite a document.
- Did a bad-path move, accepted corrected path.
- Did a good-path move.
- Did a path move with a weird component like `/???/`.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5492
Differential Revision: https://secure.phabricator.com/D10838
Summary:
- Order checks used `=` but intended `==`. We could probably write a lint rule for this.
- Selecting `*` with a join could pick (for example) `id` columns from both the document and content tables and end up using the wrong one.
- `%Q` expects a string and chokes on `null`.
Auditors: btrahan
Summary: Fixes T4666, add Herald rules to Phriction Documents
Test Plan: add Herald rule to flag if title contains "xyz", create Phriction Document with title "xyz". Phriction Document should be flagged.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4666
Differential Revision: https://secure.phabricator.com/D10830
Summary: When merging tasks, the corresponding transaction on the merged task should be black, and the transaction on the ultimate task should be green.
Test Plan: Create two tasks, merge one into the other, merged task transaction is black, the surviving task should show a green transaction.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6500
Differential Revision: https://secure.phabricator.com/D10827
Summary: when creating new documents the policy object wasn't being initialized properly. update the code to use the new handy initializeNewDocument method. Fixes T6527.
Test Plan: viewed a doc at /w/asdsadsadsdas/ and saw the correct policy setting
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6527
Differential Revision: https://secure.phabricator.com/D10837
Summary: i think way back in D10490 I didn't incorporate feedback correctly. make this code right as it fatals in this codepath as is. Fixes T6508.
Test Plan: @joshuaspence to the rescue (I remain unable to test this effectively with my baby-clean installation.)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, joshuaspence
Maniphest Tasks: T6508
Differential Revision: https://secure.phabricator.com/D10833
Summary: Fixes T6495. convert ad hoc query to a PhrictionDocumentQuery, thus enforcing view permissions
Test Plan: noted my test user a had a great wiki while test user b couldn't see most things.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6495
Differential Revision: https://secure.phabricator.com/D10822
Summary:
Ref T5833. I want to add SSH keys to Almanac devices, but the edit workflows for them are currently bound tightly to users.
Instead, decouple key management from users and the settings panel.
Test Plan:
- Uploaded, generated, edited and deleted SSH keys.
- Hit missing name, missing key, bad key format, duplicate key errors.
- Edited/generated/deleted/etc keys for a bot user as an administrator.
- Got HiSec'd on everything.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10824
Summary:
- The icon CSS tag is transformed through the new function
PhabricatorProjectIcon::getAPIName($key), which returns
a name without fa-.
- Color is a trivial lookup
- Profile image returns the PHID or null if not available
Test Plan:
- Create two projects, with different icon and color,
one with and one without profile image.
- Request information on both using project.query
Then:
[ ] Confirm icon and colors are correct for both projects
[ ] Confirm image PHID is correct
[ ] Confirm image PHID is null for the project without image
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: yuvipanda, Korvin, legoktm, epriestley
Maniphest Tasks: T6501
Differential Revision: https://secure.phabricator.com/D10823
Summary: ...how do you lock down entire areas otherwise? Fixes T6496.
Test Plan: used user 1 to create x/y that user 2 can't edit. tried to create x/y/z as user 2 and got a big ole error dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6496
Differential Revision: https://secure.phabricator.com/D10819
Summary: 'cuz the wiki don't play that no more. Fixes T6497. This is mainly important to fix an incorrect policy filtering issue where a project policy can incorrectly override a document policy. Otherwise, it makes things nice and clean.
Test Plan:
- viewed the wiki - success.
- viewed wiki document list under "index" and tried a few different queries
- grep'd for things like "hasProject" and "getProject" and saw no phriction-related results
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6497
Differential Revision: https://secure.phabricator.com/D10818
Summary: Allow `./bin/diviner generate` to continue even if there is an exception throw processing an atom. This allows Diviner documentation to be generated for PHP source code that cannot be parsed with XHPAST.
Test Plan: Ran `./bin/diviner generate` on a PHP repository which previously throw an `XHPASTSyntaxErrorException`.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10803
Summary: Fixes T5387. I broke this in D8805. Fix it by using the application search parameters that D8805 introduced.
Test Plan: verified that the two links mentioned in T5387 worked for me. Also tried manual links on secure.phabricator.com and those showed the right data even...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5387
Differential Revision: https://secure.phabricator.com/D10817
Summary:
Ref T4029. Fixes T6034.
Various front-end miscellania here. See D10814#96251. This more or less makes policy work but I am not going to call it "fixed" here since we need D10814 to be deployed too and will do that manually.
Test Plan:
- changed document policy from web ui and changes persisted
- changed document policy from web and had form error and changes persisted
- created a structure like users/users/justmyuserpolicy and made sure another user could delete the users/users/ doc
- moved a doc from a to b and verified policy persisted
- verified stub documents inherited policy of the document that stub them...!
- uploaded a file and verified that it 1) had the permissions of the page it was added to and 2) had an "attached" tab linking back to the page on the file page (this means T6034 is fixed with this)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6034, T4029
Differential Revision: https://secure.phabricator.com/D10816
Summary: Ref T4029. this diff makes the pertinent database changes AND adds the migration script. This is important to get the data backend straightened away before we fully ship T4029. Next diff will expose the edit controls for these policies and whatever else work is needed to get that part done right.
Test Plan: made sure the lone project page on my wiki had a project with restrictive view policy. Post migration verified correct policy applied to this lone project page AND most open policy applied to the others
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10814
Summary:
Ref T5833. This fixes a few weird things with this table:
- A bunch of columns were nullable for no reason.
- We stored an MD5 hash of the key (unusual) but never used it and callers were responsible for manually populating it.
- We didn't perform known-key-text lookups by using an index.
Test Plan:
- Ran migrations.
- Faked duplicate keys, saw them clean up correctly.
- Added new keys.
- Generated new keys.
- Used `bin/auth-ssh` and `bin/auth-ssh-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10805
Summary:
Fixes T1191. I'll write up the changelog with notes about this and open a feedback task for followups.
When you run `storage upgrade`, automatically run `storage adjust` afterward. Provide a flag to disable this.
This brings everyone into the utf8mb4 world.
Test Plan: Ran `bin/storage upgrade` with various flags. Ran `bin/storage adjust`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10800
Summary: Missed this in previous pass. Send these as links in HTML emails.
Test Plan: Register a new user that nees approval.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10815
Summary:
Ref T1191. Use `storage quickstart` to regenerate `quickstart.sql` using modern schema construction statements.
This puts new installs into utf8mb4 mode immediately without requiring storage adjustment.
Test Plan:
- Ran `arc unit --everything`, which uses quickstart.
- Ran `bin/storage upgrade --namespace temp`, to quickstart a new namespace.
- Ran `bin/storage upgrade --namespace temp --disable-utf8mb4`, to quickstart a new namespace without utf8mb4 support.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10797
Summary: @btrahan asked about this but I gave the wrong answer. These
currently do not turn into links. I think they might have in the past,
but if they did the rule is a little weird and feels specific to my
use. We can reexamine this at some point, but for now just make the
links work in a normal, reasonable sort of way.
Auditors: btrahan
Summary: Fixes T6262. Ref T4029. Also gets us ready for T5873 for these end points. I can file something new about someday adding phriction.query, etc but I think we'll remember and can look at that post T5873.
Test Plan: made a document via conduit and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6262, T4029
Differential Revision: https://secure.phabricator.com/D10813
Summary: Ref T4029. Even more code consolidation and cleanup for the long term benefits!
Test Plan: moved a page successfully. tried to move a page to an existing page and got an error. did the two tab trick to try to move a deleted page and got an error.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10812
Summary:
- Warn users that they'll need to be comfortable with the CLI.
- Move XHProf stuff to the developer docs, since few/no normal users need it.
Test Plan: Read documentation.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10810
Summary:
- Direct users to detailed bug report / feature reuqest documents.
- Move "get more info" and "unreproducible problems" to bug reporting document.
- Stop telling users to email us, and strongly encourage them to use primary channels.
Test Plan: Read documentation.
Reviewers: btrahan, chad
Reviewed By: btrahan, chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10808
Summary: Ref T4029. More cleanup and code consolidation for the long terms benefits.
Test Plan: found a document and opened up two browser tabs. Loaded delete dialog on both. Completed delete in one tab and noted document was properly deleted. Tried to complete delete in tab 2 and got an error message.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10809
Summary: Ref T1191. Same deal as D10786. These were previously case-insensitive, but changed to a case-sensitive column type.
Test Plan: Ran `bin/storage adjust` and got and adjustment.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: webframp, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10806
Summary: Fixes T6489. In an earlier diff I forgot to update the document status itself, thinking that the content update percolated up magically. Restore this functionality so the wiki works better.
Test Plan: deleted a document and observed that i did not get the option to delete it again and it disappeared from document index
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6489
Differential Revision: https://secure.phabricator.com/D10807
Summary:
Ref T4029. Some business logic lives outside the editor. This revision moves that logic from the edit controller into the editor proper. This makes re-using that business logic across other endpoints - say like a conduit end point - possible. This is also part of the general modernization quest for phriction I am on.
This diff also restores the functionality where you can delete a document by wiping out the content and saving.
Test Plan: tried to make a document with no title or content and saw errors. opened a document for edit with user 1, then made edits with user 2, then saw an error when i made the edit with user 1. clicking "overwrite changes" then worked. deleted a document by wiping out the body and clicking save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10795
Summary:
Ref T4029. Long live PhrictionTransactionEditor...! this means that all existing functionality runs 100% through the modern transactions + editor framework. this diff does a few things in sum
- kills the old editor
- moves conduit-based edits to new editor
- moves stubbing out documents to new editor
- deletes moving of wiki docs for projects functionality... (T4021#59511 is a better bigger battle plan here.)
Test Plan: edited a phriction document via conduit and it worked. created a new phriction document /that/was/deep/ and verified ancestral documents were properly stubbed out. changed a project name and noted no wiki page moves.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10792
Summary: Ref T5833. Since these will no longer be bound specifically to users, bring them to a more central location.
Test Plan:
- Edited SSH keys.
- Ran `bin/ssh-auth` and `bin/ssh-auth-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10791
Summary:
Ref T5833. Currently, SSH keys are associated only with users, and are a bit un-modern. I want to let Almanac Devices have SSH keys so devices in a cluster can identify to one another.
For example, with hosted installs, initialization will go something like this:
- A request comes in for `company.phacility.com`.
- A SiteSource (from D10787) makes a Conduit call to Almanac on the master install to check if `company` is a valid install and pull config if it is.
- This call can be signed with an SSH key which identifies a trusted Almanac Device.
In the cluster case, a web host can make an authenticated call to a repository host with similar key signing.
To move toward this, put a proper Query class on top of SSH key access (this diff). In following diffs, I'll:
- Rename `userPHID` to `objectPHID`.
- Move this to the `auth` database.
- Provide UI for device/key association.
An alternative approach would be to build some kind of special token layer in Conduit, but I think that would be a lot harder to manage in the hosting case. This gives us a more direct attack on trusting requests from machines and recognizing machines as first (well, sort of second-class) actors without needing things like fake user accounts.
Test Plan:
- Added and removed SSH keys.
- Added and removed SSH keys from a bot account.
- Tried to edit an unonwned SSH key (denied).
- Ran `bin/ssh-auth`, got sensible output.
- Ran `bin/ssh-auth-key`, got sensible output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10790
Summary: Ref T4214, this breaks the 'eye' out as a separate image 40px x 40px. We also now show the eye on mobile, as we have enough room for both currently.
Test Plan: Tested default and nightmaremoon colors, tested mobile, tablet and desktop layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4214
Differential Revision: https://secure.phabricator.com/D10794
Summary: Fixes T6480. @epriestley is very popular and has over 100 messages, thus triggering this issue. fix the typo because bugs are bad.
Test Plan: set limit to 1, observed fatal, applied patch and fatal went away
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6480
Differential Revision: https://secure.phabricator.com/D10793
Summary:
Fixes T2792. This adds a pluggable configuration layer between all the stuff on disk (local/file) and the runtime configurable stuff (database).
An install can subclass this source and:
- For Phacility, query a remote service (like Almanac) to retrieve hostname-based configuration, allowing one install to serve multiple instances.
- Maybe for Phacility, query a remote service (like Phlux) to retrieve sitevar-like configuration (e.g., put everything in readonly mode to deal with a maintenance issue?). Not sure if we'll do this or not. We might just nuke Phlux since Almanac is sort-of-a-superset of it for our purposes.
- For third parties, query some other remote service if that makes config management easier. In particular, it would theoretically let you put locked config in Zookeeper or whatever else you want.
Test Plan: Added a fake source and saw it inject configuration.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2792
Differential Revision: https://secure.phabricator.com/D10787
Summary:
Ref T5833. Allow services and devices to be tagged with projects.
(These fluff apply implementations are a good example of the issue discussed in T6403.)
Test Plan: {F229569}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10782
Summary:
Ref T5833. Adds support for arbitrary properites to Almanac devices and bindings.
- For Devices, this allows you to maybe mark what `rack` a server is on, the `serial` number of a router, etc.
- For Bindings, this allows you to maybe mark that a bound device is `active`, provide `credentials`, expose it as `readonly`, etc.
Test Plan: Added properties to Devices and Bindings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10781
Summary:
Ref T5833. Currently, we have an `AlmanacDeviceProperty`, but it doesn't use CustomFields and is specific to devices. Make this more generic:
- Reuse most of the CustomField infrastructure (so we can eventually get easy support for nice editor UIs, etc).
- Make properties more generic so Services, Bindings and Devices can all have them.
The major difference between this implementation and existing CustomField implementations is that all other implementations are application-authoritative: the application code determines what the available list of fields is.
I want Almanac to be a bit more freeform (basically: you can write whatever properties you want, and we'll put nice UIs on them if we have a nice UI available). For example, we might have some sort of "ServiceTemplate" that says "a database binding should usually have the fields 'writable', 'active', 'credential'", which would do things like offer these as options and put a nice UI on them, but you should also be able to write whatever other properties you want and add services without building a specific service template for them.
This involves a little bit of rule bending, but ends up pretty clean. We can adjust CustomField to accommodate this a bit more gracefully later on if it makes sense.
Test Plan: {F229172}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10777
Summary:
Ref T4029. Much like D10756, D10761 this does the bare minimum to get things in there. I have a sticky with "TODOs" about moving the error-checking business logic into the editor in all three cases.
Up next - policy...
Test Plan: moved a document and it worked! verified no feed story. verified both documents involved looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10763
Summary:
Ref T4029. Much like D10756 this does the bare minimum to get things in there. I have a sticky with "TODOs" about moving the error-checking business logic into the editor in both cases.
Up next - move actions...
Test Plan: deleted a document and it worked! verified proper feed story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: shadowhand, Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10761
Summary: Ref T1191. After adjustment, usernames currently end up case-sensitive, which means `alincoln` and `Alincoln` are different users. Make them case-sensitive so these names collie.
Test Plan: Ran `bin/storage adjust`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10786
Summary: While not explicitly misleading, this document can do a better job of covering the common/modern case.
Test Plan: Read document.
Reviewers: rush898, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10783
Summary: Fixes T6316. Wraps feed content in remarkup.
Test Plan: Post a comment with a quote on Task, go to Feed and see the quote properly styled.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6316
Differential Revision: https://secure.phabricator.com/D10788
Summary: Fixes T6469. Changes the default icon into text instead. Added the text to hidden boards and now display when reordering as well.
Test Plan:
Moved a bunch of columns, tested reordering. Seems more clear.
{F229626}
{F229627}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6469
Differential Revision: https://secure.phabricator.com/D10784
Summary:
Ref T1191. Currently if a developer forgot to specify a column type, `storage adjust` aborts explosively mid-stream. Instead:
- Make this a formal error with an unambiugous name/description instead of something you sort of infer by seeing "<unknown>".
- Make this error prevent generation of adjustment warnings, so we don't try to `ALTER TABLE t CHANGE COLUMN c <unknown>`, which is nonsense.
- When schemata errors exist, surface them prominiently in `storage adjust`.
Overall:
- Once `storage upgrade` runs `storage adjust` automatically (soon), this will make it relatively difficult to miss these errors.
- Letting these errors slip through no longer escalates into a more severe issue.
Test Plan:
Commented out the recent `mailKey` spec and ran `storage adjust`:
```
$ ./bin/storage adjust --force
Verifying database schemata...
Found no adjustments for schemata.
Target Error
phabricator2_phriction.phriction_document.mailKey Column Has No Specification
SCHEMATA ERRORS
The schemata have serious errors (detailed above) which the adjustment
workflow can not fix.
If you are not developing Phabricator itself, report this issue to the
upstream.
If you are developing Phabricator, these errors usually indicate that your
schema specifications do not agree with the schemata your code actually
builds.
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10771
Summary: Fixes T3942, turns the load links into buttons.
Test Plan: Set my limit to 1, test page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3942
Differential Revision: https://secure.phabricator.com/D10775
Summary:
Fixes T6347. This refines the "contributor guide" documents to basically lock down support further. Notable changes in policy:
- Bugs: Emphasis on reproduction steps, strong emphasis on using Maniphest. Emphasis on what we support.
- Features: Emphasis on describing problems instead of solutions, emphasis on realistic expecations about timelines. Strong emphasis on using Maniphest.
- Code: Strong emphasis on coordinating with us first. No GitHub pull requests. Emphasis on us ignoring contributions we don't have time to deal with. Suggests local forks.
Test Plan: Read these through; let me generate them and take some screenshots for easier reading.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6347
Differential Revision: https://secure.phabricator.com/D10764
Summary:
Fixes an issue with T5336 / D9871. We did 99% of the work here but didn't actually turn on the priority sorting. The unit test passed by default, which didn't catch this.
- Fix the unit test (it failed).
- Fix the query (test now passes).
- Add a "Next in Queue" element to the UI to make this kind of thing easier to spot/understand.
Test Plan: Ran unit test. Viewed "Next in Queue". Queued some tasks, flushed the queue. Web UI tracked the state sensibly.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Differential Revision: https://secure.phabricator.com/D10766
Summary: Fixes T6436. We subclass the wrong controller and miss the admin-only check.
Test Plan: Ignored / unignored set up issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6436
Differential Revision: https://secure.phabricator.com/D10765
Summary: Fixes T6343. Grepped for all callsites and added addLinkSection where needed.
Test Plan: Tested Differential, Maniphest, Conpherence, Ponder and Macro. Inspect HTML mail for anchor tags. Inspect text mails for non-disruption.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: talshiri, Korvin, epriestley
Maniphest Tasks: T6343
Differential Revision: https://secure.phabricator.com/D10762
Summary:
This implements as little as possible to stick a working transactions + editor codepath in the basic create / edit flow. Aside from the transaction tables, this also required adding a mailKey to a phrictionDocument.
Future work would include adding more transactions types for things like "move" and all the pertinent support. Even future work is to add things like policies which will work easily in the transaction framework. Ref T4029.
Test Plan:
- made a wiki doc
- edit a wiki doc
- had someone subscribe to a wiki doc and edited it
For all three, the edits worked, a reasonable email was sent out, and feed stories were generated.
- made a wiki doc at a /location/like/this
document "stubs" were made as expected in /location and /location/like
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10756
Summary: Fixes T6427.
Test Plan: Log out of sandbox, navigate to public task, click 'See Details' in a transaction. Get Dialog.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6427
Differential Revision: https://secure.phabricator.com/D10759
Summary:
Ref T1191. Notable stuff:
- Adds `--disable-utf8mb4` to `bin/storage` to make it easier to test what things will (approximately) do on old MySQL. This isn't 100% perfect but should catch all the major stuff. It basically makes us pretend the server is an old server.
- Require utf8mb4 to dump a quickstart.
- Fix some issues with quickstart generation, notably special casing the FULLTEXT handling.
- Add an `--unsafe` flag to `bin/storage adjust` to let it truncate data to fix schemata.
- Fix some old patches which don't work if the default table charset is utf8mb4.
Test Plan:
- Dumped a quickstart.
- Loaded the quickstart with utf8mb4.
- Loaded the quickstart with `--disable-utf8mb4` (verified that we get binary columns, etc).
- Adjusted schema with `--disable-utf8mb4` (got a long adjustment with binary columns, some truncation stuff with weird edge case test data).
- Adjusted schema with `--disable-utf8mb4 --unsafe` (got truncations and clean adjust).
- Adjusted schema back without `--disable-utf8mb4` (got a long adjustment with utf8mb4 columns, some invalid data on truncated utf8).
- Adjusted schema without `--disable-utf8mb4`, but with `--unsafe` (got truncations on the invalid data).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10757
Summary: Fixes T6419. Also, there was a question on T6419 about whether this was in a try catch block and it is... Its not clear to me what happens in the "timeout" case though?
Test Plan: looks nice
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6419
Differential Revision: https://secure.phabricator.com/D10755
Summary:
Fixes T6416. The comment is consistent with intent, but the actual regexp doesn't quite work right. In particular, we incorrectly match `#security.` as `security.` (with a period) instead of `security` (with no period).
Since this stuff is a pain to test and I evidently got it wrong in this case in D8703, make it unit testable.
Test Plan:
Added unit tests. Also:
{F227181}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6416
Differential Revision: https://secure.phabricator.com/D10753
Summary: Ref T5833. Allows you to bind a service (like `db.example.com`) to one or more interfaces (for example, to specify a pool with one read/write host and two read-only hosts). You can't configure which hosts have which properties yet, but you can add all the relevant interfaces to the service. Next diff will start supporting service, binding, and device properties like "is writable", "is active", etc., so that Almanac will be able to express operations like "change which database is writable", "disable writes", "bring a device down", etc.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10745
Summary: Fixes T6399. This allows you to use global search to find projects by searching for text in their descriptions.
Test Plan: Added a unique word to a project description, reindexed it, searched, got a hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6399
Differential Revision: https://secure.phabricator.com/D10748
Summary: Fixes T6394. See that task for a description. See T6403 for a proposed long-term fix.
Test Plan: Mentioned a task from a dashboard panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6394
Differential Revision: https://secure.phabricator.com/D10750
Summary: Fixes T6372. Apparently ye olde error logs get some crazy spam action as is... Looking around at call sites, we do not specify $config (which could specify the supportage of message id header) so it seems correct to default this to something. I went with "true" as the spot we use this seems like pretty easy stuff that will always work.
Test Plan: lots of thinking
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6372
Differential Revision: https://secure.phabricator.com/D10749
Summary: Fixes T6395. Ref T6350. I guess I missed this code spot in prior testing / I definitely didn't run an empty commit through it. Works now though.
Test Plan: made an empty commit and observed stuck importing status and errors in phd log. applied patch and commit successfully imported with no errors. made another empty commit and it imported as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350, T6395
Differential Revision: https://secure.phabricator.com/D10746
Summary: Fixes T6386. I missed this callsite in D10698.
Test Plan: Loaded local domained blog, no fatal.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6386
Differential Revision: https://secure.phabricator.com/D10744
Summary: type "string" should be type "text". Ref T6366.
Test Plan: viewed config and saw appropriate, working example
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6366
Differential Revision: https://secure.phabricator.com/D10736
Summary: ...also truncate authorName to 255 so that we don't get database errors. Ref T6350.
Test Plan: see T6350 - mostly doing it live - but I did sanity check and commit something and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350
Differential Revision: https://secure.phabricator.com/D10734
Summary: Ref T6350. We build $sql based on whose subscribed. If no one is subscribed, then we have no query to run.
Test Plan: observed one error disappearing from my daemon log. Also, more doing it live.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350
Differential Revision: https://secure.phabricator.com/D10731
Summary: as opposed to "requested". Also re-jigger how the "reason" works so the herald editor can get more specific data rather than a generic message. Fixes T6345 along with companion diff D10726.
Test Plan: made a herald rule to add auditors to a commit and saw it work!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6345
Differential Revision: https://secure.phabricator.com/D10730
Summary: Ref T6345, This adds more consistent color choices to match how Phabricator generally works across Differential/Diffusion per user statuses.
Test Plan: Review a few Audits in my sandbox.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T6345
Differential Revision: https://secure.phabricator.com/D10726
Summary: Ref T6350. I just set up a repository to import secure.phabricator.com/P and saw an error in my error logs about this, creating a fatal around when we publish feed stories. this is late enough in the editor code path I could see it firing again and again and again...
Test Plan: the first phabricator commit ever only had one duplicated transaction before i applied this patch. its yet to do it again. otherwise, ask some users with the issue to deploy it and see if it fixes things for them too.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350
Differential Revision: https://secure.phabricator.com/D10729
Summary: Ref T4484
Test Plan: Made a mock. Made a herald rule to subscribe a user if mock had a string in title. Edited mock to have said string. Observed user subscribed correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4484
Differential Revision: https://secure.phabricator.com/D10725
Summary: Fixes T6261. The performance of asking gravatar for these images is horrible and causing lots of people to have issues with the page.
Test Plan: noted how wildly fast the edit profile picture page loaded
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6261
Differential Revision: https://secure.phabricator.com/D10724
Summary: Fixes T6336. Turns out that the function to update the import status updates that database and doesn't update the object. If the object doesn't get the pertinent update AND there's a herald rule that runs, then the object is later re-saved without ever getting the update flag.
Test Plan: logic in the ole sandbox and going to push it to prod and run re-parse on impacted commits
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, chad
Maniphest Tasks: T6336
Differential Revision: https://secure.phabricator.com/D10723
Summary: Ref T5833. An interface is an IP (maybe v4, maybe v6) and port on a specified network (public internet, VPN, NAT block, etc).
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10718
Summary: Ref T5833. This differentiates address spaces like the public internet from VPNs, so when a service is available at `192.168.0.1`, we'll know it's on some specific NAT block or whatever.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10715
Summary: Ref T5833. The "uninteresting" part of this object is virtually identical to AlmanacService.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10714
Summary: Ref T5833. See that task for functional goals and some discussion of design.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10713
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary: see title
Test Plan: set config to allow public access and viewed a hovercard uri. saw a hovercard with little info as opposed to login prompt. Fixes T6337.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6337
Differential Revision: https://secure.phabricator.com/D10722
Summary: we don't want to mention these phids... when expanding transactions, build the unmnentionable map and make it so. slightly hairy due to how the editor framework works, but overall i think this is the right place to put these hooks. Fixes T6331.
Test Plan: made a commit with a commit message that had fixes, refs, depends on, and auditors and saw no erroneous mentions
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, chad, epriestley
Maniphest Tasks: T6331
Differential Revision: https://secure.phabricator.com/D10721
Summary: Fixes T2497. I'm not sure where we are with subscribers and custom vs normal codepath, but the mailtags implementation makes no assumptions and can handle it either way.
Test Plan: made a commit and got some sensible mail tags
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T2497
Differential Revision: https://secure.phabricator.com/D10712
Summary:
Fixes T4896, T6293.
Do most of the work in the editor, but pull the raw patch in the daemon and set that on the editor. This is somewhat of a pre-optimization but it was easy enough to do and makes sense to me.
Test Plan:
made a commit and saw it get parsed.
made a commit with "Auditors: foo" field and saw audit made for foo
turned on inline patch and attach patch and saw the patches
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6293, T4896
Differential Revision: https://secure.phabricator.com/D10705
Summary: These were missing. Sorry, need to fix this interface someday.
Test Plan: pay for stuff on mobile
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10708
Summary: Fixes T6305, sets edit and view uris later in the stack.
Test Plan: Create and edit a project in /project/. Create a project in a dialog. Get redirected to correct place. Verify Cancel send you back to home.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T6305
Differential Revision: https://secure.phabricator.com/D10702
Summary: pre-patch, we match on things like https / http and port... just match domains. Fixes T5693.
Test Plan: arc diff -> arc land and the diff was closed correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5693
Differential Revision: https://secure.phabricator.com/D10701
Summary:
I am not sure how valuable this is *as is* - I think it needs different explanations for what happened in mercurial or subversion? I do not know what those explanations are.
Made an error in D10485 - the $hashes that were saved is an array of objects, so it ends up turning into garbage via the wonders of serialization and de-serialization. Fix that by explicitly saving the tree hash.
I would like to make this work for the other VCS types we support, add the "undo / nope" button and call it fixed.
Ref T3686.
Test Plan: clicked "explan why" and saw why
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5693, T3686
Differential Revision: https://secure.phabricator.com/D10489
Summary: missing a setHandles on this codepath I think...? Fixes T6300.
Test Plan: not actually tested - I just think this is the fix since the other renderX methods all do this setHandles thing and I can't figure out how handles get set otherwise...
Reviewers: epriestley, avivey
Reviewed By: epriestley, avivey
Subscribers: avivey, Korvin, epriestley
Maniphest Tasks: T6300
Differential Revision: https://secure.phabricator.com/D10699
Summary: Fixes T6301. Just missed this in building out the page.
Test Plan: Viewed an initiative, verified `pageObjects` populated correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6301
Differential Revision: https://secure.phabricator.com/D10700
Summary: Default $phids to array() and update it if getValue() has something pertinent... Fixes T6292.
Test Plan: just used the ole logic noodle on this one.
Reviewers: chad, epriestley
Reviewed By: chad, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6292
Differential Revision: https://secure.phabricator.com/D10697
Summary: Ref T5702. This primarily gets URI routing out of Aphront and into an Application, for consistency.
Test Plan: Loaded some pages, got static resources.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10696
Summary: Ref T5702. Primarily, this gets the custom DarkConsole URI routes out of the Aphront core and into an Application, like almost all other routes.
Test Plan: Used DarkConsole.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10695
Summary: Ref T2787. When order statuses change, send merchants and users email about it.
Test Plan: Used `bin/mail` to review mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10694
Summary: Ref T2787. I mostly just want these in place so I can glue emails to them, but they're also useful on their own.
Test Plan: {F216515}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10688
Summary: Ref T2787. Currently, we show all orders/charges, which won't scale well. Show the 10 most recent and link to full order/charge history.
Test Plan: {F216325}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10685
Summary: Ref T2787. This stuff is now irrelevant and/or has no callsites.
Test Plan: `grep`, poked around
Reviewers: chad, btrahan
Reviewed By: chad, btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10684
Summary: Ref T5835. Sprinkle `shouldAllowPublic()` around to let logged-out users gain access.
Test Plan: Viewed an initiative while logged out.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10683
Summary: Ref T5835. Dump these into global search so you can find them.
Test Plan: {F216290}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10682
Summary:
Ref T2787. Make this a little more concrete with explicit membership instead of a general edit policy. In particular, we need to know who to email when orders happen, and can't reasonably do that with an edit policy.
I imagine this might eventually get more nuanced (e.g., users who can only approve orders vs users who can manage the merchant itself) but that's a long ways away.
Test Plan: {F216284}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10681
Summary:
Fixes T6145, T4016.
Filed T6287 and T6288 for some polish on this.
Test Plan: Made new projects from Maniphest - great success. Made new projects from project / create - also great success.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4016, T6145
Differential Revision: https://secure.phabricator.com/D10679
Summary:
Ref T2787.
- Account members can add and remove other members (major use case is corporate accounts).
- Use a modern edge constant setup.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10678
Summary: Ref T5835. Make fund stories publish to feed and send email.
Test Plan: Made edits, etc., saw them in feed and outbound email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10677
Summary: Ref T5835. Show backing amounts in transactions. Account for and show refunds.
Test Plan: {F215869}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10676
Summary: Ref T2787. Allow merchants to flag orders for review. For now, all orders are flagged for review. Eventually, I could imagine Herald rules for coarse things (e.g., require review of all orders over $1,000, or require review of all orders by users not on a whitelist) and maybe examining fraud data for the providers which support it.
Test Plan: {F215848}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10675
Summary: Ref T2787. Support multiple payment accounts so you can have personal vs company payment accounts.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10673
Summary:
Ref T2787. Currently, we dump the user back into the application. Instead, give them a confirmation screen and then let them continue.
Also fix a couple of unit tests I adjusted the underlying behavior of somewhat-recently in libphutil.
Test Plan: {F215498}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10672
Summary: Ref T2787. These don't necessarily do a ton yet, but we can get PayPal out of hold, at least.
Test Plan: Updated charges from all providers. Cleared a PayPal hold.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10670
Summary:
Ref T2787. When Paypal comes back to us with funds on hold, dead-end the transaction but handle it properly.
Generally, smooth out the user interaction on weird states.
Implement refudnds/cancels for Paypal.
Test Plan: {F215230}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10667
Summary: We were saying "Object Restricted Object"; instead say "Restricted Object". Fixes T6104.
Test Plan: made a restricted paste and a restricted task and saw good error messages. {F215281} {F215282}
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6104
Differential Revision: https://secure.phabricator.com/D10668
Summary:
Ref T2787. Currently, we kill a cart and dead-end the workflow on a charge failure.
Instead, fail the charge and reset the cart so the user can try using a valid payment instrument like a normal checkout workflow would.
Some shakiness/smoothing on WePay for the moment; PayPal is still made up since we don't have a "Hold" state yet.
Test Plan: {F215214}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10666
Summary: Fixes T4018. Basically hits the bullet points in that task description except the "ideally" one.
Test Plan:
ran bin/config migrate and saw sensible output.
```
~> ./bin/config migrate
Migrating file-based config to more modern config...
Skipping config of source type PhabricatorConfigDatabaseSource...
Skipping config of source type PhabricatorConfigLocalSource...
Skipping config of source type PhabricatorConfigDefaultSource...
Done. Migrated 0 keys.
```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T4018
Differential Revision: https://secure.phabricator.com/D10490
Summary:
Ref T2787. This has some rough edges but basically works.
- Users can cancel orders that are in incomplete states (or in complete states, if the application allows them to -- for example, some future application might allow cancellation of billed-but-not-shipped orders).
- Merchant controllers can partially or fully refund orders from any state after payment.
Test Plan: This is still rough around the edges, but issued Stripe and WePay refunds.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10664
Summary:
Ref T2787.
- Allow merchants to disable payment providers.
- Show more useful information about providers on the payments page.
- Make test vs live more clear.
- Show merchant status.
- Add a description to merchants to flesh them out a bit -- the merchant areas of responsibilities seem to be fitting well with accounts, etc.
Test Plan: {F215109}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10662
Summary: Fixes T6265, allows you to pass required:false as a parameter.
Test Plan: Add required:false to a field, no longer see "Required"
Reviewers: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6265
Differential Revision: https://secure.phabricator.com/D10659
Summary: Ref T2787. Uses the real icons. Straightens out the add payment flow a tiny bit.
Test Plan: {F214922}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10654
Summary: Fixes T6252
Test Plan: Test project query from conduit app, see no errors in log.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6252
Differential Revision: https://secure.phabricator.com/D10655
Summary: Ref T6256, this prevents more installs from getting in this weird state. We'll have to follow up if possible to "fix" the issue retroactively.
Test Plan: Test moving a backlog column to new position, hiding rest of other panels.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6256
Differential Revision: https://secure.phabricator.com/D10651
Summary:
Ref T2787. Builds on D10649 by rebining existing objects (carts, charges, etc) to merchantPHIDs and providerPHIDs instead of an implicit global merchant and weird global artifacts (providerType / providerKey).
Basically:
- When you create something that users can pay for, you specify a merchant to control where the payment goes.
- Accounts are install-wide, but payment methods are bound to merchants. This seems to do a reasonable job of balancing usability and technical concerns.
- Replace a bunch of weird links between objects with standard PHIDs.
- Improve "add payment method" flow.
Test Plan: Went through the Fund flow with Stripe and WePay, funding an initiative.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10652
Summary:
Ref T2787. Instead of making providers global configuration, make them a thing on merchants with web configuration.
Payment methods and some of the pyament workflow needs to be retooled a bit after this, but this seemed like a reasonable cutoff point for this diff.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10649
Summary:
Ref T2787. Currently, you add payment providers (Stripe, Paypal, etc) in global configuration.
Generally, this approach is cumbersome, limiting, and often hard for users to figure out. It also doesn't provide a natural way to segment payment receivers or provide web access to administrative payment functions like issuing refunds, canceling orders, etc. I think that stuff definitely needs to be in the web UI, and the rule for access to it can't reasonably just be "all administrators" in a lot of reasonable cases.
The only real advantage is that it prevents an attacker from adjusting settings and pointing something at an account they control. But this attack can be mitigated through notifications, some sort of CLI-only merchant lock, payment accounts being relatively identifiable, etc.
So introduce "merchants", which are basically payable entities. An individual merchant will have attached Paypal, Stripe, etc., accounts, and access rules. When you buy something in an application, the merchant to pay is also specified. They also provide an umbrella for dealing with permissions down the line.
This may get a //little// cumbersome because if there are several merchants your saved card information is not shared across them. I think that will be fine in the normal case (most installs will have only one merchant). Even if it isn't and we leave providers global, I think introducing this is the right call from a web UI / permissions point of view. I'll play around with it in the next couple of diffs and figure out exactly where the line goes.
Test Plan: Listed, created, edited, viewed merchants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10648
Summary:
Ref T2787. These were still stuck in the stone ages.
(The handles are pretty skeletal but most aren't used anywehre.)
Test Plan: Funded an initiative without anything breaking. Grepped for removed constants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10647
Summary: Ref T2787. Like Stripe, this one is pretty easy to get working correctly on the "good" path and fataling out in a safe way on bad paths.
Test Plan: Funded an initiative with Balanced.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10645
Summary:
Ref T2787. For test charges, Paypal is putting the charge in a "payment review" state. Dealing with this state requires way more infrastructure than other providers: we're supposed to pause delivery, then poll Paypal every 6 hours to see if the review has resolved.
Since I can't seem to generate normal test charges, I can't test Paypal for now. Disable it until we have more infrastructure.
(This diff gets us further along, up to the point where I hit this issue.)
Test Plan: Read documentation, rolled eyes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10644
Summary:
Ref T2787. This basically already works correctly since the hard logic is external to the provider on API providers. Tweak a couple of things.
Failures still just fail the cart completely, for now.
Test Plan: Completed a charge with Stripe.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10640
Summary:
Ref T2787. This doesn't get all the edge cases quite correct, but is generally a safe, complete payment workflow:
- Shares the actual charging state logic.
- Makes it appropriately stateful with locking and transactions.
- Gets the main flow correct.
- Detects failure cases, just tends to blow up rather than help the user resolve them.
Test Plan:
- Charged with WePay.
- Charged with Infinite Free Money.
- Resumed an abandoned cart.
- Hit all failure states where we just dead-end the cart. Not ideal, but (seemingly) complete/safe/correct.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10639
Summary: Ref T2787. Similar to D10634, give applications more control over the cart workflow. For now this just means they get to pick exit URIs, but in the future they can manage more details of cart behavior.
Test Plan: Funded an initiative and got returned to the initiative instead of dead-ending in Phortune.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10638
Summary: without escapage here, creating databases fails. Fixes T6251.
Test Plan: ran the command CREATE DATABASE foo COLLATION binary and it failed; ran the command CREATE DATABASE foo2 COLLATION "binary" and it worked; trusting that the %T still works as advertised.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6251
Differential Revision: https://secure.phabricator.com/D10641
Summary: Fixes T6254 and renames status as string. Though maybe this should go through `formatStringConstants`?
Test Plan: Reload Conduit page, see new text.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6254
Differential Revision: https://secure.phabricator.com/D10637
Summary:
Ref T2787. When a user purchases a product in Phortune, transition the cart through a purchased state and invoke product callbacks so applications can respond to the workflow.
Also shore up some stuff like preventing negative amounts of funding.
Test Plan: Backed an initiative and saw it show up on the initiative after completing the purcahsing workflow.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10635
Summary: Ref T2787. `Product` is currently a fairly heavy object, but as Phortune develops it makes a lot of sense to make it a lighter object and put more product logic in applications. Convert it into a fairly lightweight reference to applications. The idea is that Phortune is mostly providing a cart flow, and applications manage the details of products.
Test Plan: Funded an initiative for $1.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10634
Summary:
Ref T2787. Phortune currently stores a bunch of stuff as `...inUSDCents`. This ends up being pretty cumbersome and I worry it will create a huge headache down the road (and possibly not that far off if we do Coinbase/Bitcoin soon). Even now, it's more of a pain than I figured it would be.
Instead:
- Provide an application-level serialization mechanism.
- Provide currency serialization.
- Store currency in an abstract way (currently, as "1.23 USD") that can handle currencies in the future.
- Change all `...inUSDCents` to `..asCurrency`.
- This generally simplifies all the application code.
- Also remove some columns which don't make sense or don't make sense anymore. Notably, `Product` is going to get more abstract and mostly be provided by applications.
Test Plan:
- Created a new product.
- Purchased a product.
- Backed an initiative.
- Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10633
Summary: Ref T1191. We don't create new databases with appropriate collation yet.
Test Plan:
Created a new database and saw it issue:
```
>>> [10] <query> CREATE DATABASE IF NOT EXISTS `phabricator2_testo` COLLATE utf8mb4_bin
```
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10632
Summary:
Ref T4209. This creates storage for public keys against authorized hosts, such that servers can be authorized to make Conduit calls as the omnipotent user.
Servers are registered into this system by running the following command once:
```
bin/almanac register
```
NOTE: This doesn't implement authorization between servers, just the storage of public keys.
Placing this against Almanac seemed like the most sensible place, since I'm imagining in future that the `register` command will accept more information (like the hostname of the server so it can be found in the service directory).
Test Plan: Ran `bin/almanac register` and saw the host (and public key information) appear in the database.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D10400
Summary: Fixes T6119. This is a little fuzzy, but generally bumping up `innodb_buffer_pool_size` to something bigger than the default (which is often anemic, at `8M`) is desriable, and it seems like it will fix the specific issue a user encountered in T6119.
Test Plan: {F211855}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6119
Differential Revision: https://secure.phabricator.com/D10630
Summary:
Ref T1191. Although I fixed some of the mutations earlier (in D10598), I missed the column mutations under old versions of MySQL. In particular, this isn't valid:
- `ALTER TABLE ... MODIFY columnName VARCHAR(64) COLLATE binary`
Issue the permitted version of this instead, which is:
- `ALTER TABLE ... MODIFY columnName VARBINARY(64)`
Also fixed an issue where a clean schema had the wrong nullability for a column in the draft table. Force it to the expected nullability.
The other trick here is around the one column with a FULLTEXT index on it, which needs a little massaging.
Test Plan:
- Forced my local install to return `false` for utf8mb4 support.
- Did a clean adjust into `binary` columns.
- Poked around, added emoji to things.
- Reverted the fake check and did a clean adjust into `utf8mb4` columns.
- Emoji survived.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: fabe, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10627
Summary: thanks mailbox
Test Plan: unit tests
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10629
Summary:
Ref T1191. After utf8mb4 conversion, these tests no longer pass because MySQL allows emoji and gclefs and such.
We could keep these tests running by keeping a `ut8f_bin` table around somewhere, but we have no other use cases for it and it does not seem worth the added complexity. All these BMP-only codepaths are on the way out.
Update the `%s` / `%B` test to make sure it's rejecting invalid byte sequences, which are still not permitted.
Test Plan: Tests now pass.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10621
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary: Ref T1191. The index's case sensitivity depends on the column type. Using `text` makes the search case-sensitive, which is not desirable.
Test Plan: After adjustment, searched for "PROJECTS" and found hits against "projects".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10619
Summary: Fixes T6211. This gives Herald rules an explicit execution order, which seems generally good. See some discussion on T6211 and inline.
Test Plan:
- Added unit test.
- Dry ran rules and saw rules appear in the expected order in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6211
Differential Revision: https://secure.phabricator.com/D10624
Summary: Fixes T6210. The current messaging may be confusing if `pygmentize` is available but broken.
Test Plan: Faked the binary names and hit the errors, which seemed helpful.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6210
Differential Revision: https://secure.phabricator.com/D10626
Summary: Ref T6201. This isn't quite perfect but should be good enough. At some point far in the future I plan to revamp feed rendering a bit. This should possibly become a real ApplicationTransaction story eventually, too.
Test Plan: {F211777}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10625
Summary:
Ref T6223. Two issues:
- We don't use `/u` mode on these regexps. Without `/u`, the `\w`/`\W`/`\s`/`\S` modifiers have bad behavior on non-ASCII bytes. Add the flag to use unicode mode, making `\w` and `\s` behave like we expect.
- We might possibly want to do something different here eventually (for example, if the `/u` flag has some huge performance penalty) but this seems OK for now.
- We use `\b` (word boundary) to terminate the match, but `🐳` is not a word character. Use `(?!\w)` instead ("don't match before a word character") which is what we mean.
Test Plan: {F211498}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6223
Differential Revision: https://secure.phabricator.com/D10618
Summary: Ref T1191. The bulk of the slowness in T1191 is copying tables. In some cases, we can't avoid this, but we have various readthrough caches which may be very large and are safe to drop, and dropping them is very quick (much less than 1 second). In particular, dropping the `changeset_parse_cache` made the process at least ~8 minutes faster on `secure.phabricator.com` (I killed it after 8 minutes, so I'm not sure what the real number is).
Test Plan: Ran `bin/storage adjust` and saw it drop caches before applying adjustments.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10616
Summary: Ref T1191. Similar issue to D10613. This column usually has a hash exactly 12 bytes long, but sometimes stores an internal builtin query name like "open", "all", etc. It might be nice to promote those to 12-byte hashes of a consistent length eventually, but for now just make this a variable-length column.
Test Plan: Ran migration, no longer saw issues with reordering builtin saved searches.
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10614
Summary:
Ref T1191. The `bytes` types are BINARY(...), which is fixed-length and zero-pads. These hashes are not 64 characters long, so migrating them to `binary` ends up with a bunch of zero-padding.
Instead, migrate them to `text` so we drop the zero padding. It would be vaguely nice to either introduce a `varbytes` type (ick) or change the hash size to a standard size (nicer) eventually, but this isn't very important.
Test Plan: Will adjust `secure.phabricator.com`.
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10613
Summary: Ref T1191. I renamed the phases but missed these two since I didn't have any more key issues locally.
Test Plan: Ran `bin/storage adjust` in production with key issues.
Reviewers: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10612
Summary:
Ref T1191. When changing the column type of an AUTO_INCREMENT column, we currently may lose the autoincrement attribute.
Instead, support it. This is a bit messy because AUTO_INCREMENT columns interact with PRIMARY KEY columns (tables may only have one AUTO_INCREMENT column, and it must be a primary key). We need to migrate in more phases to avoid this issue.
Introduce new `auto` and `auto64` types to represent autoincrement IDs.
Test Plan:
- Saw autoincrement show up correctly in web UI.
- Fixed an autoincrement issue on the XHProf storage table with `bin/storage adjust` safely.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10607
Summary:
Ref T1191. Currently, the `quickstart.sql` gets generated in a pretty manual fashion. This is a pain, and will become more of a pain in the world of utf8mb4.
Provide a workflow which does upgrade + adjust + dump + destroy, then massages the output to produce a workable `quickstart.sql`.
Test Plan: Inspected output; I'll test this more throughly before actually generating a new quickstart, but that's some ways away.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10603
Summary:
Ref T1191. For most text columns, we either don't care if "a" and "A" are the same, or we expect them to be different (for example: keys, domains, secrets, etc). Default text columns to the `_bin` collation so they are compared by strict character value. This is safer in cases where we aren't sure.
For some text columns, we allow the user to sort by the column in the UI (like Maniphest task titles) or we do care that "A" and "a" are the same (for example: project names). Introduce a new class of virtual data types, the "sort..." types, to cover these columns. These are like the "text..." types but use sorting collations which treat "A" and "a" the same.
Test Plan:
- Made an effort to identify all columns where the UI relies on database collation.
- Ran `bin/storage adjust` and cleared all warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: beng, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10602
Summary:
Ref T1191. These are a bit tricky because keys can interact with column changes, so basically we do three phases:
1. Nuke all bad keys.
2. Make all column (and database/table) changes.
3. Fix all nuked keys.
Test Plan: Ran migration locally. See note for remaining issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10599
Summary:
Ref T1191. Adds a new workflow which can apply schema adjustments.
For now, it only performs database and table collation/charset adjustments. I believe these are extremely safe/minor, because they only affect the default values for newly created columns.
Test Plan:
- Ran migration on various database states, database/table changes went through cleanly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10595
Summary:
Ref T1191. This was useful for annotating everything but we no longer need it; there are just two types of issues now:
- Error: stuff we can't fix (missing or surplus tables/database/columns, bad column nullability).
- Warning: stuff we can fix (column types, character sets, collations, missing or surplus keys, incorrectly defined keys, bad key uniqueness).
Test Plan: Saw 3,399 warnings and 0 errors.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10594
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary:
Ref T1191. We have several keys on `<x, y, id>`. When `id` is an auto-increment primary key, I believe this is exactly equivalent to a key on `<x, y>`, because the leaf nodes are implicitly sorted by `id`. We omit the implicit `id` elsewhere.
It would be nice to drop the `id` bit for consistency, but it's not doing any harm and this doesn't need to block the primary work of T1191.
Test Plan: Saw slightly fewer warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10592
Summary:
Ref T1191. This destroys surplus columns:
- Pholio's transaction comments have a `mockID` column, but this is not used. The `imageID` column is used instead.
- Phragment has an unused `description` column.
- Releeph has an unused `summary` column.
Test Plan:
- Grepped for usage of these columns.
- Checked that these exist in production, too.
- Ran upgrades.
- Added Pholio inline comments.
- Saw fewer warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10591
Summary:
Ref T1191.
- Adds definitions for missing keys and keys with wrong uniqueness. Generally, I defined these before fixing the key query to actually pull all keys and support uniqueness.
- Moves "key uniqueness" to note severity; this is fixable (probably?) and there are no remaining issues.
- Moves "Missing Key" to note severity; missing keys are fixable and all remaining missing keys are really missing (either missing edge keys, or missing PHID keys):
{F210089}
- Moves "Surplus Key" to note seveirty; surplus keys are fixable all remaining surplus keys are really surplus (duplicate key in Harbormaster, key on unused column in Worker):
{F210090}
Test Plan:
- Vetted missing/surplus/unique messages.
- 146 issues remaining.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10590
Summary:
Ref T1191. Notable:
- Drops a very old saved query table. See comments inline: plan was to remove it after a year. It's been ~a year and two weeks.
- This has our only fulltext index. I'm not supporting that formally for now, but left a note.
- This has our only MyISAM table. I'm not supporting that explicitly for now, but it shouldn't affect anything. I may deal with this in the future.
- These tables don't actually write directly via Lisk, so there's some fiddling to get the schemata right.
Test Plan: Down to ~250 warnings. No more surplus databases or tables.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10589
Summary:
Ref T1191. Notable:
- `HeraldApplyTranscript` is not actually a DAO and has no table (it is serialized into HeraldTranscript).
Test Plan: Down to fewer than 300 issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10588
Summary:
Ref T1191. Nothing too notable here:
- Allow a Lisk object to specify that there's no expectation that a table exists. We have one Harbormaster object and one Token object like this.
- Removed BuildPlanTransactionComment because it's currently unused.
Test Plan:
- Saw ~200 fewer warnings; just ~800 left.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10583
Summary:
Ref T1191.
- Removes ponder comment table. This was migrated a very long time ago.
Test Plan:
- Grepped for removed table.
- Saw ~100 fewer issues in web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10582
Summary:
Ref T1191. Notes:
- Drops the project affiliation table. This is a very old membership table which was migrated to edges.
- Drops the subproject table. This is a very old table for a removed feature.
Test Plan:
- Grepped for dropped tables.
- Saw ~100 fewer setup issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10581
Summary:
Ref T1191. Some notes here:
- Drops the old LDAP and OAuth info tables. These were migrated to the ExternalAccount table a very long time ago.
- Separates surplus/missing keys from other types of surplus/missing things. In the long run, my plan is to have only two notice levels:
- Error: something we can't fix (missing database, table, or column; overlong key).
- Warning: something we can fix (surplus anything, missing key, bad column type, bad key columns, bad uniqueness, bad collation or charset).
- For now, retaining three levels is helpful in generating all the expected scheamta.
Test Plan:
- Saw ~200 issues resolve, leaving ~1,300.
- Grepped for removed tables.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10580
Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit.
Test Plan:
- Published a Differential feed story into Asana with comment text.
- Pulbished a Diffusion feed story into Asana with comment text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10584
Summary: Ref T1191. This actually works without T1191, but makes emoji use on the desktop easier.
Test Plan: {F210416}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10605
Summary: See <https://github.com/phacility/phabricator/issues/665>. From reading documentation, this seems dramatically better for InnoDB tables than the default behavior.
Test Plan: Ran `bin/storage dump`, got a reasonable-looking dump.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10606
Summary:
Ref T1191.
- This drops two tables.
- Both tables were migrated to transactions a very long time ago and no longer have readers or writers.
Test Plan: Saw ~150 fewer warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10576
Summary: Fixes T6184. On a Revision page we don't show the date as an important piece of information, so it's also not likely useful on a Hovercard (and confusing as to what the date means).
Test Plan: Hover over a linked Diff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6184
Differential Revision: https://secure.phabricator.com/D10579
Summary:
An explicit navigation markup was recenty added. Use it in
the userguide instead of ad-hoc -> or `->` chains.
Test Plan: Read the docs.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10560
Conflicts:
src/docs/user/userguide/diffusion_hosting.diviner
Summary: Fixes T6199, checks if Calendar is installed and displays if so.
Test Plan: Turned Calendar on and off, tested both layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6199
Differential Revision: https://secure.phabricator.com/D10574
Summary: Fixes T6189. We currently don't raise these to the editor level, so files, mentions, and project stuff get ignored.
Test Plan: Verified that files added to question and answer bodies end up attached to the relevant objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6189
Differential Revision: https://secure.phabricator.com/D10564
Summary: Ref T6185. Although it seems that we can't easily defuse or mitigate this, we can at least warn administrators.
Test Plan: Ran on my (unpatched, local) system, got a setup warning.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6185
Differential Revision: https://secure.phabricator.com/D10561
Summary: Take my secrets on the road
Test Plan: View Passphrase on mobile device, see action list.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10562
Summary:
Ref T2015. This increases the Drydock worker lease time to 24 hours. We noticed that some leases took longer than 2 hours when leasing from AWS (the actual resource was successfully leased at around 2 hours, 19 minutes).
24 hours should be plenty enough time to actually lease anything from EC2 (or any other leases during builds).
Test Plan: Have not yet tested this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D10544
Summary: This fixes a unit test failure that started occurring due to the new membership locking feature.
Test Plan: Ran the unit tests.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10546
Summary: Ref T1191. Handful of minor things here (T6150, T6149, T6148, T6147, T6146) but nothing very noteworthy.
Test Plan: Viewed web UI, saw fewer errors.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10527
Summary: Fixes T6177. Now that we've reframed "Beta" into "Prototype", there's no reason this needs to be in a separate super-hidden class of application anymore.
Test Plan: Saw Releeph available as a normal Prototype application.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6177
Differential Revision: https://secure.phabricator.com/D10550
Summary: Fixes T6176. Language here is a bit awkard but I wanted to use the verb "removed" *and* still have the object first, so I ended up adding the before details parenthetically.
Test Plan: story no longer fatal'd in my feed
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T6176
Differential Revision: https://secure.phabricator.com/D10549
Summary: Fixes T6169 by using the new nav element on the existing troubleshooting hint the user missed. Fixes T6173 by implementing the user's suggestion.
Test Plan: Looked at docs and they looked good.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: fas, epriestley, Korvin
Maniphest Tasks: T6169, T6173
Differential Revision: https://secure.phabricator.com/D10548
Summary: Fixes T5374. Add an acceptance test to the `PhabricatorInfrastructureTestCase` class which fails if a Celerity map is not up-to-date. In order to achieve this, a lot of code used to generate Celerity maps was transferred from `CelerityManagementMapWorkflow` to `CelerityResourceMap` and `CelerityResourceMapGenerator`.
Test Plan: Ran `arc unit` and noticed that all tests passed. Modified a JavaScript file and ran `arc unit` again (without running `./bin/celerity map`)... this time the test failed, as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5374
Differential Revision: https://secure.phabricator.com/D9817
Summary:
Ref T1191.
- Adds support for custom fields.
- Adds support for partial indexes (indexes on a prefix of a column).
- Drops old auxiliary storage table: this was moved to custom field storage about a year ago.
- Drops old project table: this was moved to edges about two months ago.
Test Plan:
- Viewed web UI, saw fewer issues.
- Used `grep` to verify no readers/writers for storage or project table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10526
Summary:
Ref T1191. Three parts:
- The old way of getting key information only got primary / unique / foreign keys, not all keys. Use `SHOW INDEXES` to get all keys instead.
- Track key uniqueness and raise warnings about it.
- Add a new "all issues" view to show an expanded, flat view of all issues. This is just an easier way to get a list so you don't have to dig around in the hierarchical view.
Test Plan:
{F206351}
{F206352}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10525
Summary:
Ref T1191.
- There was a varchar(50) column. I changed it to `text64`, since this length is unusual.
- There was an int(3) column. I changed it to `int32`, since this length is unusual.
Test Plan: Ran migrations, saw warnings disappear from config tool.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10524
Summary: Ref T1191. This was migrated to transactions a very long time ago.
Test Plan: Ran migration, grepped, left comments in Slowvote.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10523
Summary: T1191. Nothing very notable here.
Test Plan: Saw more blue in web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10522
Summary: Ref T1191. Nothing too exciting in these.
Test Plan: Saw more blue in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10521
Summary:
Ref T1191. Notable:
- Allowed objects to remove default columns (some feed tables have no `id`).
- Added a "note" severity and moved all the charset stuff down to that to make progress more clear.
Test Plan:
Trying to make the whole thing blue...
{F205970}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10519
Summary: Ref T1191. Fills in some more of the databases. Nothing very notable here. I didn't encounter any issues or overlong keys.
Test Plan: Used web UI to click around and verify expected schemata match up against actual schemata well.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10516
Summary:
Fixes T5603. Puts the toggling of locking membership into the editor so we get exceptions and all that.
I think the dialogue when you try to leave a project that is locked could be a little better maybe? Right now it just says "You can't leave" and "The membership is locked" more or less; should I surface a link to the policy stuff there too?
Test Plan:
- made a project, toggled the "lock" setting, observed stickiness and good transactions being made
- locked a project and tried to leave as a non-editor - got a dialogue letting me know i couldn't
- locked a project and tried to leave as an editor - left successfully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5603
Differential Revision: https://secure.phabricator.com/D10508
Summary: Ref T1191. This fills in some more features and gets audit and auth nearly generating reasonable expected schemata.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10500
Summary:
Ref T1191. The major issue motivation here is that InnoDB keys have a maximum length of 767 bytes. When we move `utf8` colums to `utf8mb4` columns, they'll jump from 3 bytes per character to 4 bytes per character, which may make some indexes too long. Add key schema to help spot this.
Also add nullability since it doesn't hurt.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10499
Summary:
Ref T1191. This lays some groundwork for generating the expected schemata, so we can compare them to the actual schemata and produce a meaningful diff.
- In general, each application will subclass `PhabricatorConfigSchemaSpec` and provide a definition of the tables it expects.
- This class has helper methods to mostly-automatically build table definitions for Lisk and (in the future) edges.
- When building expected schema, we specify a "data type", like "epoch". This is the type of data the application stores in the column, from the application's point of view. The SchemaSpec converts this into the best avilable storage type: for example, "text" will translate to `utf8mb4` if it's availalbe, or `binary` if not. This gives us a layer of indirection to insulate us from craziness.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10497
Summary:
Ref T1191. This builds on the "view of the database as it exists" by building a view of the database as it is expected to exist (this is mostly empty for now) and comparing the two. We now render a view of the "comparison schema", which is the actual schema merged with the expected schema and annotated with the differences.
(I'm merging them like this because it makes it easier to handle both "missing" and "surpulus" warnings in a consistent way. If we tried to annotate just the actual or expected schema, the absence of components which are expected to exist is messy to handle.)
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10496
Summary:
Ref T1191. Plan here is:
- Build a tool showing the current schemata status (this diff).
- Have it compare the current status to the desired status (partly here, mostly in future diffs).
- Then add a migration tool, and eventually a setup issue to tell people to run it.
Test Plan:
Reviewed current schemata.
{F204492}
{F204493}
{F204494}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10494
Summary: See rP8806fb0296c2.
Test Plan:
me fail english
with bonus!
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10514
Summary:
Fixes T6084. Changes:
- Rename `phabricator.show-beta-applications` to `phabricator.show-prototypes`, to reinforce that these include early-development applications.
- Migrate the config setting.
- Add an explicit "no support" banner to the config page.
- Rename "Beta" to "Prototype" in the UI.
- Use "bomb" icon instead of "half star" icon.
- Document prototype applications in more detail.
- Explicitly document that we do not support these applications.
Test Plan:
- Ran migration.
- Resolved "obsolete config" issue.
- Viewed config setting.
- Browsed prototypes in Applications app.
- Viewed documentation.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6084
Differential Revision: https://secure.phabricator.com/D10493
Summary: ...also re-jiggers all the anchor stuff to use $xaction ID. This seemed like the simplest way once I got in the code, as well as having nice properties for if / when we want to re-add some ajax stuff since the ID is a pretty solid piece of data to key off. Fixes T6083.
Test Plan: mentioned DX in private DX+1. Could see on DX the mention as me and not as the other user. For transactions, I left a comment on Paste and it worked, and I edited an existing transaction and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6083
Differential Revision: https://secure.phabricator.com/D10488
Summary: Fixes T5536. Some bonus pht in there.
Test Plan: made a diff hovered over the stars and saw my new text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5536
Differential Revision: https://secure.phabricator.com/D10487
Summary:
Implements a new transaction - still TYPE_ACTION - but using a new DifferentialAction::ACTION_COMMIT_CLOSE. Augment rendering as necessary to display this new transaction. Saves enough information so T3686 is possible but stops short of implementing a popup to display this information. Fixes T5875. Ref T3686.
One small display oddity - this new transaction now renders at the top of the transaction group whereas when it was a comment it was on the bottom. I think this is basically okay but if not how fix? (Playing with the "strength" of these actions will mess up the email too?)
Test Plan: made a diff X that fixed task Y. committed. checked diff X, task Y, and the commit pages for proper transactions and all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3686, T5875
Differential Revision: https://secure.phabricator.com/D10485
Summary:
Ref T5835. This is still completely made up (no payment integration), but you can "back" an initiative, type a number in the box, and generate a database row. You can then seach for backers and things you've backed and such.
Notable changes:
- Renamed "FundBacking" to "FundBacker". The former name was sort of because you can back things multiple times, but stuff like `$backings` was just too weird.
- I think that's it?
Test Plan:
- Backed an initiative.
- Viewed that I became a backer.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10486
Summary:
Ref T5835. This is all pretty boilerplate, and does not interact with Phortune at all yet.
You can create "Initiatives", which have a title and description, and support most of the expected infrastructure (policies, transactions, mentions, edges, appsearch, remakrup, etc).
Only notable decisions:
- Initiatives have an explicit owner. I think it's good to have a single clearly-responsible user behind an initiative.
- I think that's it?
Test Plan:
- Created an initiative.
- Edited an initiative.
- Changed application policy defaults.
- Searched for initiatives.
- Subscribed to an initiative.
- Opened/closed an initiative.
- Used `I123` and `{I123}` in remarkup.
- Destroyed an initiative.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10481
Summary:
Fixes T6044. We've had two cases (both the same install, coincidentally) where pages got hung doing too much data fetching.
When pages hang, we don't get a useful stack trace out of them, since nginx, php-fpm, or PHP eventually terminates things in a non-useful way without any diagnostic information.
The second time (the recent Macros issue) I was able to walk the install through removing limits on nginx, php-fpm, php, and eventually getting a profile by letting the page run for several minutes until the request completed. However, this install is exceptionally technically proficient and this was still a big pain for everyone, and this approach would not have worked if the page actually looped rather than just taking a long time.
Provide `debug.time-limit`, which should give us a better tool for reacting to this situation: by setting it to a small value (like 10), we'll kill the page after 10 seconds with a trace, before nginx/php-fpm/php/etc can kill it uselessly. Hopefully that will be enough information to find the issue (generally, getting a trace has been 95% of the problem in the two cases we've encountered).
Test Plan: Set this option to `3` and added a sleep loop, saw a termination after 3 seconds with a useful trace.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: csilvers, joshuaspence, epriestley
Maniphest Tasks: T6044
Differential Revision: https://secure.phabricator.com/D10465
Summary: Fixes T6052. Allow installs to link to legal documents, etc., in the page footer.
Test Plan:
- Configured a footer.
- Viewed workboards (no footer).
- Viewed Conpherence (no apparent disruption, I think everything z-indexes over the footer).
- Viewed stuff on mobile (seems OK).
- Viewed login page (saw footer).
{F201718}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6052
Differential Revision: https://secure.phabricator.com/D10466
Summary: Fixes T6059.
Test Plan: Made a comment on TX mentioning TX and TX+1. TX did not get a "mentioned" transaction while TX+1 did.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6059
Differential Revision: https://secure.phabricator.com/D10464
Summary: Fixes T5368. Synchronizes the page title to reflect unread counts in the notification and Conphernece messages menus.
Test Plan: {F201083}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5368
Differential Revision: https://secure.phabricator.com/D10457
Summary: Some versions of PHP aren't very happy about both interfaces and superclasses defining a method. Just remove it from MentionableInterface and leave it as implicit.
Auditors: btrahan
Summary:
Fixes T5979. There are three issues here:
- We cache document positions when you pick an item up, but don't recalculate them after you scroll, so they get out of date. Dirty the cache when the user scrolls.
- When we rebuild the cache during a drag (previously, this never happened), the position of the object you're dragging is computed wrong (since it has been moved to be under the cursor). Adjust the effective position of the object you've picked up to put it back in the right place in the list.
- When you fiddle around at the bottom of a column you can get jumpy redraws as the height adjusts. Put `min-height` on the container during a drag to prevent this.
Test Plan: In Safari, Chrome and Firefox, dragged items around on columns before and after scrolling the workboard panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5979
Differential Revision: https://secure.phabricator.com/D10455
Summary: Fixes T4036. Now if you say something on diff X like "This reminds me of Tx and Dy and commitHashFoo and Px." each of those objects gets a little visible transaction that the mention occurred. No feed, email, or notifications.
Test Plan: made a comment like above and verified transactions. also submitted a diff that "Fixes Tx" and Tx did not get the transaction as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T4036
Differential Revision: https://secure.phabricator.com/D10451
Summary:
Fixes T6056. This documentation is out of date and not very useful.
We could probably fold this option into `maniphest.priorities` at some point.
Test Plan: Read documentation, clicked link.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6056
Differential Revision: https://secure.phabricator.com/D10450
Summary: see title. Ref T5875.
Test Plan: Merged one task into another task - verified transactions on both tasks. Merged two tasks into another task - verified transactions on all three tasks. Checked out my feed and saw MERGE_INTO stories and MERGE_FROM stories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5875
Differential Revision: https://secure.phabricator.com/D10427
Summary:
Ref T6013. I accidentally made this cost explosviely huge when fixing macros for logged out users in D10411.
Specifically, we'd load all the macros, which would load all the files, which would load all the macros (to do policy checks), which would fill out of cache I think (but maybe only some of the time?). Anyway, bad news.
Instead, only load the files if we need them.
Test Plan: Viewed macro main page, macro detail, used a macro, used a meme, edited a macro, edited audio.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
Subscribers: epriestley, spicyj
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10428
Summary:
Ref T2783. Fixes T6039.
- Provide `authorPHID` and `committerPHID` to resolve T6039.
- In message parser, store author/email strings.
- In cached results, emit author/email strings.
Test Plan: Called method with and without bypassCache. Used `reparse.php` to repopulate data on an old commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783, T6039
Differential Revision: https://secure.phabricator.com/D10424
Summary: Fixes T6037. We don't currently write the "this file is attached to such-and-such object" edge on comment edits.
Test Plan: Edited a comment, adding `{Fnnn}`. Verified file was not attached before the edit, but was afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6037
Differential Revision: https://secure.phabricator.com/D10423
Summary:
Ref T5968. Issues we've seen from users include:
- Concern about severity ("... Need Restarting").
- Reduce severity of explanatory text ("Different Config", "not severe").
- Explain consequences in more detail.
- In D10420, make "Ignore" easier to find.
- Scope language for the multi-machine case ("at least one daemon").
- Confusion about why daemons need restarting.
- Unbury the lede ("Daemons and Web Have Different Config").
- Make it clear that the root cause is a different checksum by showing the checksum. (This just hammers home that we're comparing checksums and this issue is about config checksums and we're not making it up, the checksums probably aren't that useful on their own.)
- Difficulty understanding how to proceed when restarting does not resolve the issue:
- Call out steps to take on the daemon console explicitly.
- Walk through troubleshooting PHABRICATOR_ENV.
- Walk through troubleshooting multiple `local.json`.
Test Plan: {F199245}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5968
Differential Revision: https://secure.phabricator.com/D10421
Summary:
Ref T4331. Ref T5968. Users sometimes have trouble figuring out how to ignore issues. The option is a bit hard to spot, especially if you aren't familiar with interfaces yet.
Make it a button on the issue page itself instead.
Test Plan:
Normal issue:
{F199225}
Ignored issue:
{F199226}
Fatal issue:
{F199227}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4331, T5968
Differential Revision: https://secure.phabricator.com/D10420
Summary:
- `#phabricator` links to the project now.
- Provide contact address instead of personal addresses.
Test Plan: iiam
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10419
Summary: pre-patch, these fatal, since we overwrite $content to be just a string so methods fail later in the code. Instead, write a $content_str to keep $content as the proper data.
Test Plan: editing a document and on save it showed me the view page! (as opposed to fataling and staying on the eidt page)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10426
Summary:
Ref T6013. A very long time ago, edges were less clearly low-level infrastructure, and some user-aware stuff got built around edge edits.
This was kind of a mess and I eventually removed it, during or prior to T5245. The big issue was that control flow was really hard to figure out as things went all the way down to the deepest level of infrastructure and then came back up the stack to events and transactions. The new stuff is more top-down and generally seems a lot easier and cleaner.
Consequently, actors are no longer required for edge edits. Remove the parameter.
Test Plan: Poked around; ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10412
Summary:
Fixes T6013. Old image macros/memes never had the file edge written.
We also never wrote file edges for audio.
Finally, the meme controller didn't allow public access.
Write edges for images and audio, perform a migration to populate the historic ones, and make the Editor keep them up to date going forward.
Test Plan:
- Updated image, saw new image attach and old image detach.
- Updated audio, saw new audio attach and old audio detach.
- Ran migration.
- Viewed memes as a logged-out user.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10411
Summary:
Ref T6013. Currently, when we create a thumbnail, it gets its own (default) file visibility policy.
In particular, this causes the issue in T6013: thumbnails get "all users" visibility, which does not include logged-out users.
Instead, a thumbnail should just have the same visibility as the original file does. Enforce this:
- When loading thumbnails, reject thumbnails with invisible originals.
- When filtering thumbnails, permit thumbnails with visible originals.
Test Plan: As a logged-out user, thumbnails are now visible when the original files are attached to visible objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10410
Summary: Fixes T6011. See that task for discussion. We can detect when `memory_limit` will be the limiting factor for drag-and-drop uploads and warn administrators about it.
Test Plan: Fiddled configuration values and hit, then resolved, the issue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6011
Differential Revision: https://secure.phabricator.com/D10413
Summary: Fixes T6001. We currently don't allow empty secrets, but accounts with no password are occasionally used in the wild.
Test Plan:
- Created a credential with an empty secret.
- Revealed secret, saw empty message.
- Edited it (no form changes), saw secret unchanged.
- Changed it to a nonempty secret.
- Revealed nonempty secret.
- Edited it (no form changes), saw secret unchanged.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6001
Differential Revision: https://secure.phabricator.com/D10414
Summary: Fixes T5982. Probably. I'm just guessing here but like 95% sure this will fix it and 99% sure it won't hurt/break anything.
Test Plan: Still works on my 64-bit install, for what little that's worth.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5982
Differential Revision: https://secure.phabricator.com/D10415
Summary: Fixes T5993. Now that we have a context menu we can make some edit operations easier to access.
Test Plan: Toggled column visibility. Verified board state (columns shown/hidden, ordering) was retained.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5993
Differential Revision: https://secure.phabricator.com/D10417
Summary: make it use the value of the revision before any post-commit magic has occurred. Fixes T4754
Test Plan: made a herald rule that said "if revision exists, and revision accept does not exists, block push". tried to push a commit that had a revision that wasn't accepted and I was blocked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: mbishopim3, epriestley, Korvin
Maniphest Tasks: T4754, T4574
Differential Revision: https://secure.phabricator.com/D10393
Summary:
Ref T2374. Fixes T5988.
Keep track of what's been killed and not been killed, and surface that maybe you need sudo if things don't get killed with --force
...also basically make this force thing work. I managed to convinced myself stuff was getting killed with --force when it mostly wasn't. Make sure the --force parameter gets pushed as low as it needs to go to have things get killed.
Test Plan:
- `sudo ./bin/phd restart`
- `rm -rf /var/tmp/phd/pid/*`
- `./bin/phd stop` --> get warning about rogue daemons
- `./bin/phd stop X` --> get warning about no running daemons
- `./bin/phd stop --force` --> get warning about not being able to kill daemons
- `sudo ./bin/phd stop --force` --> kill daemons successfully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2374, T5988
Differential Revision: https://secure.phabricator.com/D10386
Summary: Ref T6031. I figure its totally cool to include the user creating the task as a subscriber, even if from the template case, so just do that there too. Code is written such that if the user wasn't already in the subscriber case they end up being the last person in the tokenizer. Theoretically this should make any users who didn't want to be automagically subscribed via the create from template case to remove themselves.
Test Plan: made a template from a task that didn't have me as a subscriber initially and observed i was a subscriber.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6031
Differential Revision: https://secure.phabricator.com/D10408
Summary: Fixes T6029. We should append custom fields last so they show up after things like projects, tokens, etc that render via UI events.
Test Plan: viewed a task with custom fields and projects was last
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6029
Differential Revision: https://secure.phabricator.com/D10407
Summary:
Ref T2783. This populates the following fields in DiffusionQueryCommitsConduitAPIMethod using DiffusionLowLevelCommitQuery when `bypassCache` is set to true:
* `authorName`
* `authorEmail`
* `committerName`
* `committerEmail`
* `message`
* `hashes`
The original outline called for `authorPHID` and `committerPHID` as well (but no `message` field). As far as I can tell, the PHIDs aren't actual a property on `DiffusionCommitRef`, and since the intention of this is to be able to populate a `DiffusionCommitRef`, I haven't included them. Let me know if we really do need the PHIDs here.
Test Plan: Tested using 3 Phabricator instances (one web, one taskmaster and one storage). The web and taskmaster tiers are directed at the Conduit API of the storage tier. Made a `diffusion.querycommits` from the Conduit app on the web tier instance and saw the data populated from the raw VCS data (located on the storage tier).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10399
Summary:
Fixes T5956. We changed the default mail encoding to `quoted-printable` to fix delivery via SendGrid via SMTP, but this broke multiple other mailers.
- Change the default back to 8bit (which works everywhere except SendGrid).
- Add a configuration setting for selecting `quoted-printable`.
- Document this issue.
- Discourage use of SendGrid in documentation.
(IMPORTANT) @klimek @nickz This reverts the `quoted-printable` fix for SendGrid. You will need to adjust your configurations (set `phpmailer.smtp-encoding` to `quoted-printable`) and restart your daemons or mail will get double newlines again.
Test Plan:
- Sent mail via SendGrid with various `phpmailer.smtp-encoding` settings, saw mail arrive with specified encoding.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: klimek, nickz, epriestley
Maniphest Tasks: T5956
Differential Revision: https://secure.phabricator.com/D10397
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary: purple != violet, and in our CSS we call these things by the fanciest of terms. Fixes T5995.
Test Plan: flagged something purple and saw that the "remove purple flag" flag was indeed purple. quickly tested other colors and they all seem good too.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T5995
Differential Revision: https://secure.phabricator.com/D10389
Summary: we did some security lock down on URI stuff and I think this was a casualty. Fixes T5992.
Test Plan: left a comment, got redirected. no more 500 response.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5992
Differential Revision: https://secure.phabricator.com/D10388
Summary:
Ref T5405.
- `--limit` wasn't actually used anywhere.
- Make it mean "the N newest lines".
Test Plan: Ran `bin/phd log`, `bin/phd log --limit 3`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5405
Differential Revision: https://secure.phabricator.com/D10385
Summary:
Resolves T5987. This build step was at some point converted to use yielding, which meant that whenever the build step executes it will create a new log. This checks to see if there is an existing log before creating a new one and uses that instead.
Long term we're going to need some way of attaching data to `PhabricatorWorkerYieldException` that can be read when the build step starts again; this will allow us to move more build steps off `while (...) { ... sleep(X); }` loops and onto yielding.
Test Plan: Tested locally.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T5987
Differential Revision: https://secure.phabricator.com/D10383
Summary: Fixes T4387.
Test Plan: Setup a mercurial repository for rabbitmq-server. Browsed around it and things looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4387
Differential Revision: https://secure.phabricator.com/D10380
Summary: Looks like I missed this when implementing custom actions and hence you can't currently use custom actions on the pre-commit adapters.
Test Plan: Added a custom action to a pre-commit Herald rule.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10316
Summary: Ref T1049. This messages is always printed to standard error now that the known hosts file is set to /dev/null. This hides the warning so that we'll be able to parse stderr for Windows hosts (where Powershell decides to output XML...)
Test Plan: Tested locally and verified the warning no longer appears.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10362
Summary: Ref T1049. Because we no longer destroy artifacts when builds are restarted, we need the build generation number to be part of the artifact key, otherwise we get collisions when restarting builds that contain build steps that emit artifacts.
Test Plan: Ran it with a build plan of "Lease Host" and "Run Command", no longer got an artifact key crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10336
Summary: This prevents crashes when looking at builds, where the build steps have been deleted on the build plan since the build was run. Currently the only information that's pulled from the build step is the description (because this was too large to copy to every target).
Test Plan: Tested it locally.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10361
Summary: Ref T2374. While building D10367 I noticed that phd was finding rogue daemons way more than it should be. Re-jigger this code path so rogue daemons are checked for *after* we've dealt with known daemons. This keeps the logic pretty simple overall.
Test Plan: phd start; kill pid files; phd stop and get the right warning; phd stop --force and it kills the rogue demons. phd stop in normal conditions no longer reporting rogue daemons erroneously
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2374
Differential Revision: https://secure.phabricator.com/D10368
Summary: D10281 upgraded us to modern infrastructure but I think forget to set this little helper to return true. Fixes T5975.
Test Plan: paged through notifications with glee
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5975
Differential Revision: https://secure.phabricator.com/D10369
Summary: Shows the UI everywhere. Also asort() the keys before calculating the environment hash as that is probably an issue for someone at some point we just don't need to have. Ref T5968.
Test Plan: Viewed the setup check and saw a link to the daemon console. Viewed the daemon console and saw the various stale config daemons. Clicked a daemon and saw a "stale config" header icon where expected. Restarted daemons and all of this went away.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5968
Differential Revision: https://secure.phabricator.com/D10367
Summary:
Resolves T5817. Continuation of D10231.
This corrects the rendering of the "user answered question" transaction so that it does not incorrectly attempt to render the question handle as HTML in emails if the rendering target is not HTML.
Test Plan: Used `bin/mail show-outbound` to verify that the email didn't contain escaped HTML when answering a question.
Reviewers: #blessed_reviewers, btrahan, epriestley
Reviewed By: #blessed_reviewers, btrahan, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5817
Differential Revision: https://secure.phabricator.com/D10319
Summary: I derped on this; the SFTP interface doesn't have setWorkingDirectory because it implements DrydockFilesystemInterface and not DrydockCommandInterface. So when you use the Upload File build step, the daemon will crash due to an undefined method.
Test Plan: Tested on my live server.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10351
Summary: This fixes the ZIP controller redirect in Phragment after the external redirect change.
Test Plan: Tested it on my server.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D10350
Summary: Fixes T5958
Test Plan: i just used the ole logic noodle on this one
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5958
Differential Revision: https://secure.phabricator.com/D10359
Summary:
Fixes T4057. This sort of sidesteps the trickiest (but very rare) case of things like embedded slowvotes. We might be able to refine that later.
In the common bad case (macros, large images) it gets reasonable results by using `overflow: hidden` with `max-height`.
We use `PhabriatorMarkupEngine::summarize()` to try to just render the first paragraph.
Test Plan: {F195093}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4057
Differential Revision: https://secure.phabricator.com/D10355
Summary: Fixes T2564. See screenshot.
Test Plan:
{F194796}
- Made a bunch of valid and invalid adjustments here and verified that the branches table showed autoclose state and branches consistent with the settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2564
Differential Revision: https://secure.phabricator.com/D10349
Summary: Fixes T4769. This is silly and just scratches an itch, but do a better job with navigation sequences.
Test Plan: {F195082}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4769
Differential Revision: https://secure.phabricator.com/D10353
Summary: Ref T5936. This implements build implementations aborting early when the build has since been restarted. Build steps now periodically poll to see if the build's current generation does not match their generation, and they throw a `HarbormasterBuildAbortedException` if that is the case.
Test Plan: Tested locally on my machine with the sleep build step.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5936
Differential Revision: https://secure.phabricator.com/D10322
Summary:
Fixes T4767. I believe 80% of this was actually caused by the author issue fixed in T5771, but this should help make the other 20% debuggable.
- Record why we didn't autoclose a commit when we process it.
- Show branch autoclose status in the main branch table.
- Show commit autoclose status on the edit screen.
- Add documentation about how to find these statuses and what they mean.
Test Plan:
- Read documentation.
- Viewed branches and hovered over the various states.
- Viewed commits in various states and checked the "Autoclose?" field.
- Pushed some commits and saw autoclose activate.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767
Differential Revision: https://secure.phabricator.com/D10348
Summary: Fixes T2605. Provide some instructions on configuring RDS properly. The "DB Parameter Group" thing in the web UI seems pretty easy to use, it's just not obvious that it's what you should be using.
Test Plan: Jiggled these warnings to trigger them, viewed the output, saw a table of values and a hint about RDS.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2605
Differential Revision: https://secure.phabricator.com/D10343
Summary: Ref T992. This makes HTML mail layout more consistent with text mail layout and fixes my greatest annoyance with it.
Test Plan: Used `bin/mail list-outbound --id <id> --dump-html` to view mail in Safari, saw it have a normal amount of whitespace between sections.
Reviewers: btrahan, talshiri, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D10344
Summary: Ref T5847.
Test Plan: Used `bin/remove destroy` to destroy a question. Saw the question and its answers get wiped out.
Reviewers: btrahan, shadowhand
Reviewed By: shadowhand
Subscribers: shadowhand, epriestley
Maniphest Tasks: T5847
Differential Revision: https://secure.phabricator.com/D10345
Summary:
Ref T2605. For old MySQL, this option is not supported. Catch that and tailor the error.
I couldn't find the first version of MySQL which introduced this optino in order to produce a more useful error. I spent about ~10 minutes looking.
Test Plan: Faked the error, survived setup.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2605
Differential Revision: https://secure.phabricator.com/D10342
Summary: Add a note about running this manually for troubleshooting.
Test Plan: Read it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10341
Summary: be more aggressive about assuming plain-text, use remarkup for no extension, .remarkup, and .md, and last but not least use rainbow for .rainbow. Fixes T5818.
Test Plan: my README rendered just fine post these changes
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: asherkin, epriestley, Korvin
Maniphest Tasks: T5818
Differential Revision: https://secure.phabricator.com/D10340
Summary: Fixes T4881.
Test Plan: made a config change, saw the issue, restarted daemons and it went away
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4881
Differential Revision: https://secure.phabricator.com/D10339
Summary: Fixes T5943. We do this differently in different places; Audit / Differential do something like this while Pholio expands the "byLine" to include a timestamp. Go with the Audit / Differential approach, as presumably having the date as a top line, easily scannable metadata is the goal here.
Test Plan: viewed a list of pastes and saw a timestamp of creation at the top.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T5943
Differential Revision: https://secure.phabricator.com/D10338
Summary: Ref T1049. Set the working directory when executing commands on Drydock hosts. Without this set, they execute in the user's default home directory.
Test Plan: Ran a build and saw the correct working directory when running `pwd`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: CanadianBadass, epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10293
Summary:
Fixes T5916. Key insight here is that the screenshot shows a custom "Detail Solution / Notes" field, which is why this mojo doesn't work: custom remarkup fields don't emit their content for mention/file extraction.
Also fix a bug where multiple blocks with file PHIDs could be merged improperly, discarding some file PHIDs.
Test Plan: Added a custom remarkup field, added files to it, saw them attach to the task when changes were saved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5916
Differential Revision: https://secure.phabricator.com/D10335
Summary: $email => $e_email. Fixes T5933.
Test Plan: Added an email that was already on another account and got the proper "Duplicate" UI with the duplicate email address still entered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5933
Differential Revision: https://secure.phabricator.com/D10334
Summary:
Fixes T5900. We have some very old code here which does not let you update your password if the `account.editable` flag is set.
This was approximately introduced in D890, and I think it was mostly copy/pasted at that point. I'm not sure this ever really made sense. The option is not documented as affecting this, for example. In the modern environment of auth providers, it definitely does not make sense.
Instead, always allow users to change passwords if the install has a password provider configured.
Test Plan:
- Set `account.editable` to false.
- Used a password reset link.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5900
Differential Revision: https://secure.phabricator.com/D10331
Summary: Fixes T5942. These are external but currently unmarked.
Test Plan: Visited link, got redirected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5942
Differential Revision: https://secure.phabricator.com/D10332
Summary:
Primarily, this fixes searching for `F123` in global search.
The info URI is now a better URI than the "best" URI for files, and doesn't have redirect issues.
Test Plan: Searched for `F123` in global search.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10330
Summary:
Fixes T5912. When we write files, we attempt to share storage if two files have the same content.
In some cases, we may not share storage. Examples include:
- Files migrated with `bin/files migrate` (it's simpler not to try to dedupe them).
- Old files, from before storage was sharable (the mechanism did not exist).
- Files broken by the bug fixed in T5912.
Add a script to compact files by pointing files with the same content hash at the same file contnet.
In the particular case of files broken by the bug in T5912, we know the hash of the file's content and will only point them at a file that we can load the data for, so this fixes them.
Compaction is not hugely useful in general, but this script isn't too complex and the ability to fix damage from the bug in T5912 is desirable. We could remove this capability eventually.
Test Plan:
- Ran `files compact --all --dry-run` and sanity checked a bunch of the duplicates for actually being duplicates.
- Migrated individual files with `files compact Fnnn --trace` and verified the storage compacted and all files survived the process.
- Verified unused storage was correctly destroyed after removing the last reference to it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10327
Summary:
Fixes T5926. Fixes T5830. Ref T4767. Users currently sometimes have a hard time understanding repository update frequencies. This is compounded by aggressive backoff and incorrect backoff while importing repositories.
- Don't back off while importing repositories. This prevents us from hanging at 99.99% for inactive repositories while waiting for the next update.
- Back off less aggressively in general, and even more gradually during the first 3 days. This should make behavior around weekends better.
- Show update frequency in the UI.
- Provide an explicit "update now" button to call `diffusion.looksoon` in a more user-friendly way.
- Document how backoff policies work and how to adjust behavior.
Test Plan:
- Ran `bin/phd debug pulllocal` and verified backoff worked correctly from debugging output.
- Clicked "Update Now" to get a hint, reloaded page to see it update.
- Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767, T5830, T5926
Differential Revision: https://secure.phabricator.com/D10323
Summary:
Fixes T5934. If you hash a password with, e.g., bcrypt, and then lose the bcrypt hasher for some reason, we currently fatal when trying to figure out if we can upgrade.
Instead, detect that the current hasher implementation has vanished and let the user reset their password (for account passwords) or choose a new one (for VCS passwords)>
Test Plan:
Account password:
- Artifically disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Used password reset workflow to change password, saw iterated MD5 hashed password get set.
- Enabled bcrypt hasher again.
- Saw upgrade warning.
- Upgraded password to bcrypt.
VCS password:
- Artificially disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Reset password.
- Saw iterated md5 password.
- Reenabled bcrypt.
- Upgraded to bcrypt.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5934
Differential Revision: https://secure.phabricator.com/D10325
Summary: Ref T4284. This fixes at least one problem which can cause the observed behavior.
Test Plan:
- Before applying patch, used `PHABRICATOR_CREDENTIAL=PHID-CDTL-... bin/ssh-connect` + debugging prints to verify the keyfile was written and cleaned up normally.
- Destroyed the credental, verified the temporary file was not cleand up correctly.
- Applied patch, verified temporary file was not written and command exited with sensible error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4284
Differential Revision: https://secure.phabricator.com/D10328
Summary: Clean up some arg handling stuff.
Test Plan: Used this while debugging.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10314
Summary: This was broken by rP5ac36e8 by a derpy typo.
Test Plan: Ran dry run against a revision with a a repository, saw the field fill in on the transcript.
Reviewers: nickz, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10326
Summary:
Additional audit states were made queryable for T5871.
Include them in Conduit's audit.query as well. In doing so corrects
references from "status-foo" to "audit-status-foo".
Depends on D10271
Test Plan: with an api, issues queries and got sensible results
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10290
Summary:
Ref T5932. Ref T5936. This implements build generations in Harbormaster, which provides the infrastructure required to both show users the previous states of restarted builds and to allow users to forcefully abort builds (and their targets).
You can view previous generations of a build by adding `?g=<n>` to the URI, but this isn't exposed in the UI anywhere yet.
Test Plan: Ran a build plan with a Sleep step in it. Reconfigured it for various sleep times and viewed previous generations of the build after restarting it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5932, T5936
Differential Revision: https://secure.phabricator.com/D10321
Summary:
Resolves T5937. HTTPS redirects caused by `security.require-https` use a full scheme, domain and port in the URI. Consequently, this causes invocation of the new external redirect logic and prevents redirection from occurring properly when accessing the HTTP version of Phabricator that has `security.require-https` turned on.
I've also fixed the automatic slash redirection logic to add the external flag where appropriate.
Test Plan: Configured SSL on my local machine and turned on `security.require-https`. Observed the "Refusing to redirect" exception on master, while the redirect completed successfully with this patch.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5937
Differential Revision: https://secure.phabricator.com/D10318
Summary: Caught this with the new redirect validation logic. The `$return_uri` was being set as just `B123` which is not valid. Prefixing it with `/` (like is done in `HarbormasterBuildActionController` already) gives the correct result of reloading the buildable's page.
Test Plan: Restarted all builds on a buildable, saw the page reload correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10320
Summary:
Fixes T5912. When migrating files, we try to clean up the old data. However, this code isn't aware of reference counting, and unconditionally destroys the old data.
For example, if you migrate files `F1` and `F2` and they have the same data, we'll delete the shared data when we migrate `F1`. Then you'll get an error when you migrate `F2`.
Since this only affects duplicate files, it primarily hits default profile pictures, which are the most numerous duplicate files on most installs.
Test Plan:
- Verified that the theory was correct by uploading two copies of a file and migrating the first one, before applying the patch. The second one's data was nuked and it couldn't be migrated.
- Applied patch.
- Uploaded two copies of a new file, migrated the first one (no data deletion), migrated the second one (data correctly deleted).
- Uploaded two copies of another new file, `bin/remove destory'd` the first one (no data deletion), then did it to the second one (data correctly deleted).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10312
Summary: Ref T5915. Make `bin/remove destroy` a bit more thorough, since Herald transcripts can have field information in them.
Test Plan: Used `bin/remove destroy` to nuke revisions, saw their transcripts vanish too.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5915
Differential Revision: https://secure.phabricator.com/D10306
Summary: Fixes T5915. Occasionally, users derp up and diff private key material. Adding a pre-write Herald phase enables configuration of a partial layer of protection that will reject these changes before they hit disk, provided they can be detected by, e.g., filename.
Test Plan:
- Added a rule with checks on every field, verified they looked fine in the transcript.
- Created some revisions to test those changes (I have a bunch of revision rules locally).
- Verified rejects don't write transcripts to the database.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5915
Differential Revision: https://secure.phabricator.com/D10305
Summary:
If daemon data is mangled, `bin/phd restart` will SIGINT process `0`, which kills it.
uh oh T.T so sad
Test Plan: Used `bin/phd start` to start daemons; removed PID information from one; saw `bin/phd stop` shut down cleanly and not kill itself.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mholden, epriestley
Differential Revision: https://secure.phabricator.com/D10308
Summary: and for bonus, finesse some URIs a tad. Fixes T5922.
Test Plan: viewed F1 logged out and it worked! viewed the ugly URI for F1 and got redirected to the pretty URI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5922
Differential Revision: https://secure.phabricator.com/D10309
Summary: its not necessary. Fixes T5906
Test Plan: clicked "Login to Comment" and went straight to the login form, sans intermediary dialogue
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5906
Differential Revision: https://secure.phabricator.com/D10295
Summary: Fixes T5918.
Test Plan: Verified memes work again.
Reviewers: hach-que, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5918
Differential Revision: https://secure.phabricator.com/D10307
Summary: Ref T5894. We have a couple more similar cases. Make them all do a decision-based redirect for now.
Test Plan: Did "View Raw File" and such, and also made sure thumbnails still work.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10301
Summary: Straightforward (this is the one object type we do let you delete from the web UI) implemetation of `PhabricatorDestructibleInterface`.
Test Plan: Used `bin/remove destroy` to destory several files. Used `--trace` to verify they wiped file data.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10300
Summary: This has never been enabled by default, and isn't safe. Remove it since people can use preallocated or EC2 hosts.
Test Plan: Removed it; didn't see it appear on the "Create Blueprint" page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10287
Summary: Fixes T5894. This needs some improvement when we lay in real CDN stuff, but should get all the cases right for now.
Test Plan: Thumbnails work properly again.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10299
Summary: Fixes T5798. We basically weren't using the caching mechanism. Also adds service calls for S3 stuff, and support for seeing a little info like you can for conduit.
Test Plan: uploaded a paste, looked at paste list - no s3 service calls. edited the paste, looked at paste list - no s3 service calls and edited content properly shown
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5798
Differential Revision: https://secure.phabricator.com/D10294
Summary: Fixes T5871. These queries get to use the actual column on the commit table since they are about the "aggregate" state of different audits.
Test Plan: issues queries and got sensible results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5871
Differential Revision: https://secure.phabricator.com/D10271
Summary:
Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com".
- Reject local URIs with backslashes (we never generate these).
- Fully-qualify all "Location:" redirects.
- Require external redirects to be marked explicitly.
Test Plan:
- Expanded existing test coverage.
- Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction).
- Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected).
- Went through some local redirects (login, save-an-object).
- Verified file still work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10291
Summary: Ref T2787. This provides a purchase detail screen (which has nothing useful on it yet) and converts a bunch of PHIDs into slightly more useful links.
Test Plan: Browsed around my account.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10284
Summary: Fixes T5471.
Test Plan: Used `bin/remove destroy` to destroy a dashboard and a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5471
Differential Revision: https://secure.phabricator.com/D10283
Summary: Ref T5891. This just modernizes infrastructure.
Test Plan: Viewed "All" and "Unread" notifications.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5891
Differential Revision: https://secure.phabricator.com/D10281
Summary:
Fixes T5140. When you ajax-edit a task and we send back a full-size card, we currently always put a drag grip on it.
If you clicked the "edit" thing from a priority-ordered list, this is appropriate. However, if you clicked it from some other type of list, it is not.
Pass the expected grippableness through the call.
Test Plan:
- Edited a task from a reorderable (priority-ordered) view, got grip.
- Edited a task from a nonreorderable (author-ordered) view, got no grip.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5140
Differential Revision: https://secure.phabricator.com/D10282
Summary:
Fixes T5885. This implements optional soft point limits for workboard columns, per traditional Kanban.
- Allow columns to have a point limit set.
- When a column has a point limit, show it in the header.
- If a column has too many points in it, show the column and point count in red.
@chad, this could probably use some design tweaks. In particular:
- I changed the color of "hidden" columns to avoid confusion with "overfull" columns. We might be able to find a better color.
- UI hints for overfull columns might need adjustment.
(After T4427, we'll let you sum some custom field instead of total number of tasks, which is why this is called "points" rather than "number of tasks".)
Test Plan:
{F190914}
Note that:
- "Pre-planning" has a limit, so it shows "4/12".
- "Planning" has a limit and is overfull, so it shows "5 / 4".
- Other columns do not have limits.
- "Post-planning" is a hidden column. This might be too muted now.
Transactions:
{F190915}
Error messages / edit screen:
{F190916}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10276
Summary:
Ref T5885. See D10276.
Currently, ActionHeaders can only have minicons, and we don't use them anywhere and they probably don't make much sense in the product anymore.
Instead, allow them to have font icons. Remove minicons, which have no callsites and probably won't in the future.
Test Plan:
{F190925}
- Grepped for `minicons`.
- Grepped for `setHeaderIcon()`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10277
Summary: Fixes T5884. Macro images are no longer public on most installs. We could generate tokens for them, but this (using Conduit to pull the file data) is easier and more correct.
Test Plan: Logged a bot into IRC and had it spam part of a macro before being killed for flooding.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10274
Summary: Ref T5884. We migrated with "canCDN" and then had live writes with "cancdn". Move everything to "canCDN" for consistency.
Test Plan: Ran migration, verified DB only has "canCDN" afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10273
Summary:
Ref T992.
- Format text/HTML bodies explicitly in `bin/mail show-outbound`.
- Provide `bin/mail show-outbound --dump-html` so you can do something like `bin/mail show-outbound --dump-html > dump.html; open dump.html` to get a browser preview somewhat easily.
Test Plan: Ran `bin/mail show-outbound` with and without `--dump-html` flag.
Reviewers: talshiri, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D10272
Summary:
When enabled, this will show the full history of review comments in an
email-compatible threading-view.
Test Plan: Sending emails with the option on and off.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10146
Summary: Ref T4427. This always counts 1 task = 1 point. The tricky bit is making this update in JS.
Test Plan: {F190900}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D10275
Summary:
Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Test Plan:
Been running this in our deployment for a few months now.
====Well behaved clients====
- Gmail
- Mail.app
====Bad clients====
- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.
====Need testing====
- Outlook (Windows + Mac)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: webframp, taoqiping, chad, epriestley, Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D9375
Summary: Fixes T5575. Moves "All" links into title/header. Mark all read floats left, and connection status sits in footer. Also added hints to enable notifications (it's a cool feature).
Test Plan:
Tested locally both menus.
{F190630}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5575
Differential Revision: https://secure.phabricator.com/D10269
Summary: Fixes T5453.
Test Plan: made a remarkup comment that "Q1 is dumb and Q10 is awesome" and only Q10 was linked. changed the new setting to have the value " " and the Q1 also started linking.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5453
Differential Revision: https://secure.phabricator.com/D10270
Summary: Ref T5862. makes the exception work better
Test Plan: issued some queries from audit ui with and without repos - they worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10268
Summary: Fixes T5862. The Diffusion table uses `id` but all the other infrastructure uses `phid` so just do a quick load of the repositories to get the ids. Long term, we should re-key the table by phid I think.
Test Plan: made a query with a repository and got a proper result set
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10245
Summary: Should help with debugging. In the web UI there is now a link to "View Email". This uses the same debugging output that `./bin/mail show-outbound` uses. Fixes T5768. Code is very defensive as I think these tables may be truncated eventually?
Test Plan: viewed some emails and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10244
Summary:
Fixes T5727. Updates the regexes to split on '-'. Also changes the editor such that tokens are updated by the larger search process. (Note this means we update this data more often then we need to - for every project transaction.)
Users will need to make an edit to a project -or- run `bin/search index "#project-tag"` to make this actually work.
Test Plan: Made "Frontend-Engineering", "Engineering", and "Backend-Enginering". They all showed up in the typeahead!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5727
Differential Revision: https://secure.phabricator.com/D10247
Summary: Fixes T5872. This won't show up in the initial email until T4896 is further along.
Test Plan:
```
RECIPIENTS
discoball (Disco Ball)
BODY
epriestley added a comment.
ffkn
USERS
epriestley (Author)
COMMIT
http://local.aphront.com:8080/rPOEMS165b6c54f487
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5872
Differential Revision: https://secure.phabricator.com/D10266
Summary: Fixes T5883. The first time we hit an error we'll continue forward; we only bail after the second time. Instead, check for an error immediately
Test Plan: HA HA HA DID NOT TEST HA HA HA HA
Reviewers: btrahan, cburroughs
Reviewed By: cburroughs
Subscribers: epriestley
Maniphest Tasks: T5883
Differential Revision: https://secure.phabricator.com/D10265
Summary:
Ref T5884. We migrated to add a `canCDN` flag, but the code looks for a `cancdn` flag.
If this fixes the issue, I'll migrate `cancdn` to `canCDN` in the next diff.
Test Plan: Viewed some files, including old files, and saw the cacheability I expected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10264
Summary: Fixes T4130. Adds a setup warning when ft_min_word_len is set to the default value.
Test Plan: Hit setup warning; resovled setup warning. Searched for "DOS".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4130
Differential Revision: https://secure.phabricator.com/D10259
Summary:
Fixes T3347. We can't really do this one as a config thing since we don't know if the user wants to use LDAP.
Instead, just give them a better message than they otherwise get when they try to install/configure/use LDAP.
Test Plan: Faked it and got a reasonable message.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3347
Differential Revision: https://secure.phabricator.com/D10260
Summary:
Fixes T2605.
- Add a setup warning about the stopword file.
- Provide a simpler stopword file.
Test Plan:
- Hit setup warning.
- Resolved it according to instructions.
- Added "various" to a task, then searched for it, found the task.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2605
Differential Revision: https://secure.phabricator.com/D10258
Summary: Fixes T3173. This doesn't actually fix T3173 but I'm going to redirect that. It does make the bot quit IRC gracefully, with a nicer quit message, which can be customized.
Test Plan: Got a bot to quit IRC with nice messages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3173
Differential Revision: https://secure.phabricator.com/D10257
Summary: Fixes T2101. When viewing an image change, show image dimensions, MIME type, and filesize.
Test Plan:
{F190189}
{F190190}
very utility
such wow
Reviewers: mailson, btrahan, chad
Reviewed By: chad
Subscribers: epriestley, Korvin, aran
Maniphest Tasks: T2101
Differential Revision: https://secure.phabricator.com/D5206
Summary: Fixes T4973. For `PhabricatorProjectInterface` objects, add a header to let clients do mail filtering.
Test Plan: Saw `X-Phabricator-Projects: <#goat_farm>` in outbound mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: rush898, epriestley
Maniphest Tasks: T4973
Differential Revision: https://secure.phabricator.com/D10256
Summary: Fixes T1922. When a story is about a primary object associated with projects, show those projects on the feed story.
Test Plan: {F190171}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1922
Differential Revision: https://secure.phabricator.com/D10255
Summary: Fixes T5579. Modern browsers aggressively autofill credentials, but at least Firefox still behaves slightly better with this flag. Hopefully other browsers will follow suit.
Test Plan: Browsed various interfaces, verifying that login interfaces allow autocomplete while non-login interfaces do not.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5579
Differential Revision: https://secure.phabricator.com/D10253
Summary: Fixes T5869. Ref T4896. This `setID()` method no longer exists.
Test Plan: (WARNING) This is a pain to reproduce locally so I'm just winging it. I'm 99% sure this ID is only used to generate an anchor link. This is a hack to start with, and T4896 will eventualy clean it up properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896, T5869
Differential Revision: https://secure.phabricator.com/D10254
Summary: This prevents SSH from saving the host key into known_hosts; StrictHostKeyChecking only prevents it from prompting for unknown hosts, but it will still verify hosts against what it has previously saved.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10252
Summary:
Resolves T5831. This modifies the Drydock SSH interface to execute commands under Powershell when the target host platform is Windows. Powershell is far more featured than cmd.exe, and more closely resembles a UNIX shell.
Currently Powershell outputs stderr as an XML blob on a line, and while this code currently doesn't use that, it will allow us in the future (planned next week) to redirect that output to the stderr log instead of having it all merged in with stdout under cmd (where there is no way to distinguish it).
Test Plan:
Ran various native commands and PowerShell commands from a Harbormaster build, including things like:
```
Write-Host ("my test" + ${build.id})
```
and saw:
```
my test679
```
in the output.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5831
Differential Revision: https://secure.phabricator.com/D10248
Summary: Fixes T5769. Fixes T5861. Add mail tags for "unblock" and "column change".
Test Plan: Did unblocks and column changes, verified the mail got the right mailtags and recipient nondelivery flags.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861, T5769
Differential Revision: https://secure.phabricator.com/D10241
Summary:
Ref T5861. Ref T5769. If users don't care at all about something, allow them to ignore it.
We have some higher-volume notifications either built now (column changes) or coming (mentions) which users might reasonably want to ignore completely.
Test Plan:
Ignored some notifications, then took appropraite actions. Saw my user culled from the notification subscriber list.
{F189531}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5769, T5861
Differential Revision: https://secure.phabricator.com/D10240
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.
This has zero impact on the UI or behavior.
Test Plan:
- Checked/unchecked some options, saved form.
- Swapped back to `master` and saw exactly the same values.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10238
Summary: Ref T5861. Adds an option to opt out of all notification email. We'll still send you password resets, email verifications, etc.
Test Plan:
{F189484}
- Added unit tests.
- With preference set to different things, tried to send myself mail. Mail respected preferences.
- Sent password reset email, which got through the preference.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: rush898, epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10237
Summary:
Ref T5861. These two options are complex, rarely useful, and not directly related to controlling what mail you receive.
Move them to a separate panel to make way for more stuff on the preferences panel. We'll probably add an "HTML" option to this new panel eventually, too.
Test Plan:
{F189474}
- Used both panels.
- Tested with multiplexing off.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10236
Summary:
Fixes T5185. The fundamental issue is that this `excludePHIDs` property was not saved, so the logic went like this:
- Generate `excludePHIDs` correctly.
- Pass `excludePHIDs` through the stack.
- Perform some other computations correctly.
- Queue the mail for the daemons, throwing it away. {icon bomb}
- Daemons process mail with empty `excludePHIDs` list.
Store it in the persistent properties array instead.
Also remove the "override self mail" thing, since it's only used by `bin/mail send-test` and suffers from the same issue. I think it's too useless to fix, since even if you get caught by it, `bin/mail` makes it clear why the message was dropped.
Test Plan:
Notable:
- `exclude` present in properties
- Exclusion reason under RECIPIENTS header
{P1229}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10234
Summary:
Fixes T5456. We lost this logic in the transition to applicationtransactions.
When publishing a feed story, mark all of the object's projects as related, so the project filter in feed works.
Test Plan: Made a comment on a task associated with a project, saw the story in filtered feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: timor, epriestley
Maniphest Tasks: T5456
Differential Revision: https://secure.phabricator.com/D10233
Summary:
Fixes T5233.
- The mail adapter API currently expects plain addresses (like `a@b.com`) in `addTos()`, and some adapters can not accept fancy verbose addresses (like `"name" <a@b.com>`).
- When we try to send error email, we pass the entire "From" header into the API. This is incorrect.
- Since it would be nice to make this just work in the future, fix it inside the API.
- Specifically, this is reached with: send email -> generates error -> we try to send you an email back -> we send it to your "From" -> some mailers choke on the fancy name if you have one.
Test Plan: Processed an errorneous email with a fancy "From", got a response error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5233
Differential Revision: https://secure.phabricator.com/D10232
Summary: Ref T5817. This just fixes the markup in emails, the overall behavior still isn't great. I don't want to spend to much time on Ponder until it ends up somewhere nearer the top of the priority queue.
Test Plan: Viewed feed stories and emails, no stray/clearly-broken HTML.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5817
Differential Revision: https://secure.phabricator.com/D10231
Summary: Fixes T5859. This doesn't change much, but makes the transaction record a little more accurate and activates stuff like `#hashtags` and `{F123}` causing policy associations.
Test Plan: Used `bin/mail receive-test` and mail receiver script to send bug mail, saw hashtags imply projects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5859
Differential Revision: https://secure.phabricator.com/D10229
Summary:
Fixes T5839. If a repository has been force pushed and garbage collected, we might have a ref cursor in the database which still points at the old commit (which no longer exists).
We'll then run a command like `git log <new hash> --not <old hash>` to figure out which commits are newly pushed, and this will bomb out because `<old hash>` is invalid.
Instead, validate all the `<old hash>` values before we try to make use of them.
Test Plan:
- Forced a repository into a bad state by mucking with the datbase, generating a reproducible failure similar to the one in T5839.
- Applied patch.
- `bin/repository update <callsign> --trace` filtered the bad commit and put the repository into the right state.
- Saw new commits recognized correctly.
- Ran `bin/repository update <callsign>` for a Mercurial and SVN repo as a sanity check.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5839
Differential Revision: https://secure.phabricator.com/D10226
Summary:
Fixes T5184. Fixes T5008. Three issues with stories/notifications about changing the status of tasks which block other tasks:
**Bad Feed Stories**
- Problem: Feed story rendering was confusing (T5184).
- Solution: fix it to provide context.
**Too Many Feed Stories**
- Problem: Feed gets a story for the original task's close ("a closed x"), and a story for each blocked task ("a closed x, a task blocking y").
- "Solution": Punt. These are redundant in the full feed but not in filtered feeds. Right solution is display-time aggregation. No users have really complained about this.
**Too Many Notifications**
- Problem: Users subscribed to both tasks get notified about the clsoe, and also about the unblocked task. These notifications are redundant.
- "Solution": Punt. This is easy to fix by silencing notifications for the sub-editor, but I'm worried it would be confusing. Users haven't complained. Display-time aggregation might be a better fix.
Test Plan: {F189463}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5008, T5184
Differential Revision: https://secure.phabricator.com/D10235
Summary: Ref T5819. Implements basic icon and color filtering for projects.
Test Plan: {F189350}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5819
Differential Revision: https://secure.phabricator.com/D10230
Summary:
Fixes T5855. Adds a `--graceful N` flag to `phd stop` and `phd restart`.
`phd` will send SIGINT, wait `N` seconds, SIGTERM, wait 15 seconds, and SIGKILL. By default, `N` is 15.
Test Plan:
- Ran `bin/phd debug ...` and used `^C` to interrupt daemons. Saw graceful shutdown behavior, and abrupt termination on multiple `^C`.
- Ran `bin/phd start`, `bin/phd stop` and `bin/phd restart` with `--graceful` set to various things, notably `0`. Saw graceful shutdowns on the CLI and in the web UI. With `0`, abrupt shutdowns.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5855
Differential Revision: https://secure.phabricator.com/D10228
Summary: Resolves T5836. This automatically releases artifacts when Harbormaster builds finish (either passing or failing). This allows Harbormaster to release the Drydock leases it has for hosts.
Test Plan: Tested it with a build plan that passes and fails; tested it with lots of builds running in parallel.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5836
Differential Revision: https://secure.phabricator.com/D10208
Summary: This allows timeouts to be specified on SSH connections that Drydock makes. Used in the EC2 allocator to poll for the SSH server starting.
Test Plan: Used in EC2 allocator diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10225
Summary:
Ref T1049. This keeps track of how long a build target takes to execute in Harbormaster and displays it in the build view page. I'm not sure whether "Started" is really that useful once the target has completed?
Also, I change the name of the time taken depending on whether or not the target has completed; if it's still in progress it's called "Elapsed" and if it's completed then it's "Duration". The primary reason for this is that "Duration" sounds like post tense, whereas "Elapsed" is current tense. I'm not sure whether this is okay or not?
Test Plan: Ran a Sleep build step and saw the target dates / times appear correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: talshiri, epriestley, Korvin
Maniphest Tasks: T5824, T1049
Differential Revision: https://secure.phabricator.com/D10174
Summary: To assist with {T5245}, I have added projects back into the lipsum maniphest generator with the edge infrastructure.
Test Plan: Run the lipsum script for PhabricatorManiphestTaskTestDataGenerator and make sure it generates project data.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10202
Summary: Fixes T5850. Also fixes some logic where the wrong preempting events could be attached during a bulk query.
Test Plan: Phrequent list now shows preemption-aware times.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5850
Differential Revision: https://secure.phabricator.com/D10223
Summary:
Fixes T5848.
- Disallow tracking negative time.
- Preserve note if there's an error with the time selection.
- Show start time and duration.
- Slightly better error messages.
Test Plan: Started and stopped time. Tried to select future/negative ranges.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5848
Differential Revision: https://secure.phabricator.com/D10218
Summary:
Fixes T5837. The problem is that the hash is being recognized as a commit hash. We currently fire the object monogram rules fairly early, but there's no real reason to do this. Move them after all of the hyperlink rules:
0 PhutilRemarkupEscapeRemarkupRule
100 PhutilRemarkupMonospaceRule
150 PhutilRemarkupDocumentLinkRule
175 PhrictionRemarkupRule
<<< OLD OBJECT RULE POSITION
200 PhabricatorIconRemarkupRule
200 PhabricatorMemeRemarkupRule
200 DivinerSymbolRemarkupRule
350 DoorkeeperRemarkupRuleJIRA
350 PhabricatorYoutubeRemarkupRule
350 DoorkeeperRemarkupRuleAsana
400 PhutilRemarkupHyperlinkRule
>>> NEW OBJECT RULE POSITION
500 PhabricatorImageMacroRemarkupRule
500 CustomInlineJIRA5Rule
500 PhabricatorMentionRemarkupRule
500 CustomInlineCodeRule
1000 PhutilRemarkupDelRule
1000 PhutilRemarkupBoldRule
1000 PhutilRemarkupItalicRule
1000 PhutilRemarkupUnderlineRule
- The disadvantage of this approach is that `{F123, alt=go look at http://lol.com/ omg}` will parse the URL first, and then fail to resolve the object embed. This seems very rare / unusual.
- The advantage is that all URLs which happen to have monograms in them work.
In the future, we could refine this by separating the rules, so the embed (`{...}`) versions fired at priority 200, while the normal versions fired at priority 450. We can wait for use cases, though. This is a little messy because the same code implements both rules.
Test Plan:
- Verified example in T5837.
- Marked up object rules like `F123` (works), `[[ asdf | F123 ]]` (works), `{F123, alt=http://example.com}` (does not work).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5837
Differential Revision: https://secure.phabricator.com/D10212
Summary:
See some discussion here:
24a6eeb8d8 (commitcomment-7334892)
The `protected $properties;` storage parameter added to `ProjectColumn` is shadowed by `getProperties()` in the base class.
Although this works correctly for me, it's ambiguous and worth fixing. Make the base class methods explicit.
Test Plan: Used `grep` to find callers for both methods and renamed them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10210
Summary:
Via HackerOne. If a user adds an email address and typos it, entering `alinculne@gmailo.com`, and it happens to be a valid address which an evil user controls, the evil user can request a password reset and compromise the account.
This strains the imagination, but we can implement a better behavior cheaply.
- If an account has any verified addresses, only send to verified addresses.
- If an account has no verified addresses (e.g., is a new account), send to any address.
We've also received several reports about reset links not being destroyed as aggressively as researchers expect. While there's no specific scenario where this does any harm, revoke all outstanding reset tokens when a reset link is used to improve the signal/noise ratio of the reporting channel.
Test Plan:
- Tried to send a reset link to an unverified address on an account with a verified address (got new error).
- Tried to send a reset link to a verified adddress on an account with a verified address (got email).
- Tried to send a reset link to an invalid address (got old error).
- Tried to send a reset link to an unverified address on an account with only unverified addresses -- a new user (got email).
- Requested several reset links, used one, verified all the others were revoked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10206
Summary: Ref T2787. This is very basic and just helps me know that the data is inserting correctly.
Test Plan: {F187765}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10205
Summary:
- Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
- Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
- Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.
Test Plan:
- Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
- Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
- Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5771, T4864, T5851
Differential Revision: https://secure.phabricator.com/D10221
Summary:
Fixes T5838.
- We currently try to use a `ConduitAPIMethod` object as a string.
- We then pass that string to the parent's `__construct()` method as `$message`.
Test Plan: Uninstalled Maniphest, then tried to execute `maniphest.createtask`. Got a useful exception message instead of an error during message construction.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5838
Differential Revision: https://secure.phabricator.com/D10211
Summary:
Fixes T5849. When a new file is created, we might have to actually write the data to a storage engine, or we might be able to just point at data which is already there.
Currently, these two paths handle `$params` with different code and mild behavioral differences. Instead, have them call the same code so they get the same behavior.
Test Plan:
- Uploaded the same file multiple times to home page.
- Uploaded the same file multiple times as profile picture.
- Generated files via Diffusion.
- All the files got the expected properties, whether they were reusing data or not.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5849
Differential Revision: https://secure.phabricator.com/D10216
Summary:
Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.
The issues here are:
- If we **do** redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
- If we **don't** redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.
To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.
If this is still confusing, the next best thing I can come up with is something like this:
- Put some modulous of the timestamp in the URI.
- If the current time is within 2 seconds of the generation time, show this dialog.
- Otherwise, redirect.
That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.
Other ideas:
- Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
- Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).
I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".
Test Plan: {F188914}
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10215
Summary: Ref T5685. We've added a new `canCDN` flag to control whether or not files can be cached and delivered over a CDN. Show this flag in the UI.
Test Plan: Viewed several files, saw correct/expected UI values.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10213
Summary: See D10189. We should never hit this anymore, so clean it up.
Test Plan:
- Reloaded a board, saw everything stay where it was before the change.
- Added a new task to the project, saw it show up in backlog.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10200
Summary: Fixes T5829. This stuff is old and busted, but keep it working for now.
Test Plan: No more fatal when there are recently closed tasks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5829
Differential Revision: https://secure.phabricator.com/D10201
Summary:
Fixes T5677.
- Instead of using `sequence == 0` to mean "this is the backlog column", flag the column explicitly.
- Migrate existing sequence 0 columns to have the flag.
- Add the flag when initializing or copying a board.
- Remove special backlog logic when reordering columns.
Test Plan:
- Migrated columns, viewed some boards, they looked identical.
- Reordered the backlog column a bunch of times (first, last, middle, dragged other stuff around).
- Added tasks to a project, saw them show up in the reordered backlog.
- Initialized a new board and saw a backlog column show up.
- Copied an existing board and saw the backlog column come over.
- Tried to hide a backlog column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5677
Differential Revision: https://secure.phabricator.com/D10189
Summary: Just wanted to play with this, removes the gradient 'cards' for a flat design.
Test Plan:
Tested various apps, workboards
{F166127}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9515
Summary: This slipped through the datasource modernization stuff.
Test Plan: Used search UI.
Reviewers: rush898, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10196
Summary:
Ref T5024, T4427, T5474, T5523. Instead of separate icons in the column header for "Create Task" and "Edit Column Settings", use a dropdown menu.
- T5024 will likely add a "View Standalone" option.
- T4427 needs header space to show a count.
- T5474 likely needs "Edit Triggers..." (this seems reasonable to separate from editing the name, etc.)
- T5523 likely adds "Move all tasks..." eventually.
Test Plan: {F187414}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5523, T5474, T5024, T4427
Differential Revision: https://secure.phabricator.com/D10190
Summary: Sets layout as flush when rendering diff table or timeline in a Dialog
Test Plan: Tested each
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10194
Summary: Fixes T5739. I only got D9857 half right: the new method names are correct, but the bodies needed to change too.
Test Plan: Signed a document as an anonymous user.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5739
Differential Revision: https://secure.phabricator.com/D10191
Summary:
Via the UI adding a mailinglist for CC works, but via
the API currently it shows:
>One or more PHIDs were invalid for ccPHIDS
This removes the user validation check for ccPHIDs.
(I left it in for other things like owner since that seems
still appropriate?)
Test Plan:
used arc locally to add a mailinglist to cc
```echo '{"id": 2, "ccPHIDs": ["PHID-MLST-ohduchbv4dfimk7opt3r"]}' | arc call-conduit maniphest.update```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10193
Summary:
Ref T4807. This is probably a complete fix, but I'd be surprised if there isn't a little cleanup I missed.
When users drag tasks on a "natural"-ordered workboard, leave things where they put them.
This isn't //too// bad since a lot of the existing work is completely reusable (e.g., we don't need any new JS).
Test Plan:
- Dragged a bunch of stuff around, it stayed where I put it after dropped and when reloaded.
- Dragged stuff across priorities, no zany priority changes (in "natural" mode).
- Created new tasks, they show up at the top.
- Tagged new tasks, they show up at the top of backlog.
- Swapped to "priority" mode and got sorting and the old priority-altering reordering.
- Added tasks in priority mode.
- Viewed task transactions for correctness/sanity.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10182
Summary:
Ref T4807. This is an alternative to D10179. The problem these diffs solve is that I want to be able to reorder a column's positions without having to load the actual objects, but that's difficutl because two positions may have the same sequence number (and I think it's good that we allow that, since it makes a bunch of other stuff way easier).
Instead of using the object ID (e.g., the task ID) to reorder positions with the same sequence, use the position itself. This is a little easier, is less ambiguous if columns eventually have several types of objects, and produces a better behavior when old objects are freshly added to a board. For example, if you tag `T300` with `#project`, this new rule will push it to the top of "Backlog" while the old rule might have buried it deep. I think this behavior is desirable and more "natural".
When creating a group of new rows, we do order the batch by ID, so a group of freshly-tagged objects float to the top togehter in ID order. This seems like the most natural rule, too.
Test Plan:
- Loaded some boards with implicit objects on them (freshly tagged tasks) and saw rows create.
- Verified new rows created in the right order.
- Dragged some tasks around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10180
Summary:
Ref T4807. This doesn't actually do anything yet, but adds a dropdown menu for choosing an ordering and gets all the UI working correctly.
This also fixes a bug where column hidden state wouldn't persist across filter changes.
(I won't land this until it does something, but the next diff will probably be a mess so this seemed like a clean place to sever things.)
Test Plan:
{F187114}
- Altered sort ordering.
- Altered hidden state and filters, verified all states persisted correctly.
- Added `phlog()` to edit/create and move controllers and verified they receive sort information.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: swisspol, chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10178
Summary:
CanCDN flag indicates that a file can be served + cached
via anonymous content distribution networks.
Once D10054 lands, any files that lack the CanCDN flag
will require a one-time-use token and headers will
prohibit cache to protect sensitive files from
unauthorized access.
This diff separates the CanCDN changes from the code that
enforces these restrictions in D10054 so that the changes
can be tested and refined independently.
Test Plan: Work in progress
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: rush898, qgil, epriestley, aklapper, Korvin
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10166
Summary: Fixes T5705. This was just derp; instead of returning the duration of the first slice, return the duration of all the slices.
Test Plan: Added unit tests. Saw reasonable results in the UI.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5705
Differential Revision: https://secure.phabricator.com/D10184
Summary: Fixes T5423, "is newly created" herald rule fails on dry runs
Test Plan: Create herald "is newly created" rule, and do a dry run on an existing pholio mock, differential commit, or maniphest task. Should not return an exception.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5423
Differential Revision: https://secure.phabricator.com/D10187
Summary:
Ref T5245. This removes some hacks and activates two meaningful interactions:
- The "projects" field goes through shared code now.
- Mentioning projects in tasks using hashtags now tags them.
Test Plan:
- Viewed a task with projects.
- Viewed a task with no projects.
- Viewed a task with projects and board positions.
- Viewed a revision with projects.
- Made a `#hashtag` comment in Maniphest and got a project association.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D10177
Summary: Ref T1049. This allows build steps to create URI artifacts, which can be used to link to external builds and other resources.
Test Plan: Used a build step in an external library to test the creation of a URI artifact and verified it appeared correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10173
Summary: Resolves T5814. Ref T1049. This changes "Stop" to "Pause" in the UI (internally it's still referred to as Stop).
Test Plan: Viewed builds and saw the intended wording.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049, T5814
Differential Revision: https://secure.phabricator.com/D10172
Summary: Use cutlery icon for hilarity. Ref T5768.
Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10183
Summary: Fixes T5478. For "personal" installs use the person icon; for global use the global icon. For both providing explanatory tooltip text about what's going on. This will need to be updated if / when we start installing dashboards to other applications. Also, this query isn't 100% optimized but the major part *is* so I think its okay.
Test Plan: Installed a dashboard for personal use and verified correct icon / text showed up. Did the same for global installed dashboard...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5478
Differential Revision: https://secure.phabricator.com/D10181
Summary:
This is pretty basic allowing a user to set the
policy as a valid string ('no-one' or 'users') or
as a valid PHID. Without an explicit policy
a permissive one is set.
Test Plan:
Tested using the python-phabricator module (very basic api wrapper).
The arc cli syntax was evading me.
```import base64
from phabricator import Phabricator
phab = Phabricator()
with open('mypic.jpg') as f:
encoded = base64.b64encode(f.read())
//set no-one as viewer which really means author only?
phab.file.upload(name='mypicnoone.jpg',
data_base64=encoded,
viewPolicy='no-one')
//set a specific phid as policy in this case a project
phab.file.upload(name='mypicphid.jpg',
data_base64=encoded,
viewPolicy='PHID-PROJ-fgvvnafmhvkgn2d5a4rf')
//no set policy ends up as 'users' i.e. ('all users')
phab.file.upload(name='mypicdefault.jpg', data_base64=encoded)```
Not able to really test canCDN attribute but it should be
fine and I tried to make it all consistent with D10166
Reviewers: 20after4, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: 20after4, epriestley, Korvin
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10164
Summary: Minor correction to correct spelling of alternate (the 'n' was missing).
Test Plan: reviewer to verify correct spelling
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10175
Summary:
Currently, we'll try to publish notifications while running tests. This is at best unnecessary and at worst problematic (we don't stub out the server).
For now, just never publish them.
Test Plan: Ran unit tests with notifications enabled but the server down and didn't get a bunch of warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10171
Summary: Fixes T5735, setting up Phacility for huge financial success.
Test Plan:
opened up Safari - who logs in with Safari anyway? - and could still view a macro
could also view the list of macros
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5735
Differential Revision: https://secure.phabricator.com/D10170
Summary:
Fixes T5476. Using edges to store which objects are on which board columns ends up being pretty awkward. In particular, it makes T4807 very difficult to implement.
Introduce a dedicated `BoardColumnPosition` storage.
This doesn't affect ordering rules (T4807) yet: boards are still arranged by priority. We just read which tasks are on which columns out of a new table.
Test Plan:
- Migrated data, then viewed some boards. Saw exactly the same data.
- Dragged tasks from column to column.
- Created a task directly into a column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5476
Differential Revision: https://secure.phabricator.com/D10160
Summary: so you can see who the event is about...! Fixes T5621.
Test Plan: saw the creator of each event on /calendar/event/query/all/
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5621
Differential Revision: https://secure.phabricator.com/D10169
Summary:
The `##` on the beginning of a line is now a header.
Use more-preferred backticks.
Test Plan: nope lol
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10157
Summary:
Restores functionality for Flowdock->Chatbot adapter.
Most likely the result of API changes in the year since the original patch was contributed,
the flowdock adapter no longer worked.
This makes a few tweaks to both the base streaming adapter class and the flowdock adpater. I took care to not disturb the functionality of the campfire adapter, but I don't have any way to test it.
Test Plan: I am new here and I have no idea what to write other than sarcastic things but I'll most like amend this after review.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10168
Summary: Fixes T5773.
Test Plan: Made a poll and voted on it. Deleted it via ./bin/remove destory V1. No errors and the poll is gone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5773
Differential Revision: https://secure.phabricator.com/D10167
Summary: ...and fix an error where lines that start with ##X## are rendering incorrectly by switching to alternate syntax `X`. Fixes T5806.
Test Plan: read the docs and they looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5806
Differential Revision: https://secure.phabricator.com/D10165
Summary: this data is a little weird since its user-entered and we need to put it in a web page un-escaped for the font to load correctly. Ergo, we use a regex to make the input safe / sane, and said regex needs to support a '.'. Fixes T5810.
Test Plan: added Fixedsys Excelsior 3.01 to my system and was able to set my preference and get the new font
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: dereckson, epriestley, Korvin
Maniphest Tasks: T5810
Differential Revision: https://secure.phabricator.com/D10163
Summary: This moves artifacts and build target messages into tabs.
Test Plan: Viewed build plan, saw the tabs appear when the steps had appropriate artifacts and / or messages.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10161
Summary: This automatically hides any empty build logs from Harbormaster, so that they do not appear.
Test Plan: Viewed a build plan where the logs were empty and didn't see them appear.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10091
Summary: Currently, we just create a default "backlog" column if / when you visit a workboard for the first time. Post this patch, instead you see a blocking dialog that lets you either create the default backlog column or import columns from another project. In the case of the latter, the user gets another dialog which lets them select any project of which they are a member that also has columns in it. Note that only not hidden columns get imported. Fixes T4431.
Test Plan:
- made a new workboard and got my new dialog. made a default backlog and it worked!
- made a new workboard again and tried the import flow - it also worked.
- verified projects with no columns do not show up in import dialog
- verified project with / without columns still all show up in maniphest project typeahead
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4431
Differential Revision: https://secure.phabricator.com/D10153
Summary: Ref T4896. This was used by the old audit comment storage, which is now defunct.
Test Plan: Grepped for callsites in the codebase.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10152
Summary:
Fixes T5728. In particular:
- `/tag/XYZ/` now works as an alias for `/tag/xyz/`.
- `arc todo --project ASDF` now works as an alias for `arc todo --project asdf`.
Test Plan: Called `project.query` and visited `/tag/LBHABLHBH/`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aklapper, epriestley
Maniphest Tasks: T5728
Differential Revision: https://secure.phabricator.com/D10144
Summary: Fixes T5510. This purely reduces false positives from HackerOne: we currently rotate CSRF tokens, but do not bind them explicitly to specific sessions. Doing so has no real security benefit and may make some session rotation changes more difficult down the line, but researchers routinely report it. Just conform to expectations since the expected behavior isn't bad and this is less work for us than dealing with false positives.
Test Plan:
- With two browsers logged in under the same user, verified I was issued different CSRF tokens.
- Verified the token from one browser did not work in the other browser's session.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5510
Differential Revision: https://secure.phabricator.com/D10136
Summary:
Fixes T5509. Currently, existing sessions live on even if you change your password.
Over the course of the program, we've recieved a lot of HackerOne reports that sessions do not terminate when users change their passwords. I hold that this isn't a security vulnerability: users can explicitly manage sessions, and this is more general and more powerful than tying session termination to password resets. In particular, many installs do not use a password provider at all (and no researcher has reported this in a general, application-aware way that discusses multiple authentication providers).
That said, dealing with these false positives is vaguely time consuming, and the "expected" behavior isn't bad for users, so just align behavior with researcher expectations: when passwords are changed, providers are removed, or multi-factor authentication is added to an account, terminate all other active login sessions.
Test Plan:
- Using two browsers, established multiple login sessions.
- In one browser, changed account password. Saw session terminate and logout in the second browser.
- In one browser, removed an authentication provider. Saw session terminate and logout in the second browser.
- In one browser, added MFA. Saw session terminate and logout in the second browser.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5509
Differential Revision: https://secure.phabricator.com/D10135
Summary:
Fixes T5506. Depends on D10133. When users remove an email address or change their primary email address, invalidate any outstanding password reset links.
This is a very small security risk, but the current behavior is somewhat surprising, and an attacker could sit on a reset link for up to 24 hours and then use it to re-compromise an account.
Test Plan:
- Changed primary address and removed addreses.
- Verified these actions invalidated outstanding one-time login temporary tokens.
- Tried to use revoked reset links.
- Revoked normally from new UI panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5506
Differential Revision: https://secure.phabricator.com/D10134
Summary:
Ref T5506. This makes it easier to understand and manage temporary tokens.
Eventually this could be more user-friendly, since it's relatively difficult to understand what this screen means. My short-term goal is just to make the next change easier to implement and test.
The next diff will close a small security weakness: if you change your email address, password reset links which were sent to the old address are still valid. Although an attacker would need substantial access to exploit this (essentially, it would just make it easier for them to re-compromise an already compromised account), it's a bit surprising. In the next diff, email address changes will invalidate outstanding password reset links.
Test Plan:
- Viewed outstanding tokens.
- Added tokens to the list by making "Forgot your password?" requests.
- Revoked tokens individually.
- Revoked all tokens.
- Tried to use a revoked token.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5506
Differential Revision: https://secure.phabricator.com/D10133
Summary:
Ref T4896. Now that we have a transaction editor, we can delete a giant block of hacks.
I believe this also resolves the commit/task attachment issues @joshuaspence and @mbishopim3 mentioned.
Test Plan: Attached and detached commits and tasks.
Reviewers: btrahan, joshuaspence, mbishopim3
Reviewed By: mbishopim3
Subscribers: mbishopim3, epriestley, joshuaspence
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10138
Summary: Ref T4896. I got this logic slightly wrong when porting it over: we always want to write this relationship, to allow members of a project with an audit request against a commit to resign and get it out of their queue.
Test Plan:
- Resigned from a commit with an existing relationship.
- Resigned from a commit with no existing relationship, saw one added.
Reviewers: btrahan, joshuaspence, mbishopim3
Reviewed By: mbishopim3
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10137
Summary:
Ref T4589. We don't recognize task descriptions as remarkup blocks, so `{F...}` references in them do not get attached to the objects, and thus no policy exemption is created.
Recognize them, which activates `{F...}` and `@mentions`.
We probably have a few more of these in other applications, but it's not a big deal to clean them up as they arise.
Test Plan: Uploaded a file to a task in the description field, saw it attach and get a policy exemption.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10139
Summary:
Ref T4589. When you look at a file, we load attached objects in order to run the "you can see this if you can see any attached object" policy check.
However, right now the subquery inherits the "throw on filter" flag from the parent query. This inheritance makes sense in other cases[1], but because this is an "ANY" rule it does not make sense here. In practice, it means that if the file is attached to several objects, and any of them gets filtered, you can not see the file.
Instead, explicitly drop the flag for this subquery.
[1] Sort of. It doesn't produce wrong results in other cases, but now that I think about it might produce a less-tailored error than it could. I'll look into this the next time I'm poking around.
Test Plan:
- Viewed an "All Users" file attached to a private Mock.
- Prior to this patch, I incorrectly received an exception when the Mock was loaded. This is wrong; I should be able to see the file because the policy is "All Users".
- After the patch, I can correctly view the file, just not the associated mock.
{F127074}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: 20after4, aran, epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D8498
Summary:
Fixes T4589. This implements much better policy behavior for files that aligns with user expectations.
Currently, all files have permissive visibility.
The new behavior is:
- Files uploaded via drag-and-drop to the home page or file upload page get permissive visibility, for ease of quickly sharing things like screenshots.
- Files uploaded via the manual file upload control get permissive visibility by default, but the user can select the policy they want at upload time in an explicit/obvious way.
- Files uploaded via drag-and-drop anywhere else (e.g., comments or Pholio) get restricted visibility (only the uploader).
- When the user applies a transaction to the object which uses the file, we attach the file to the object and punch a hole through the policies: if you can see the object, you can see the file.
- This rule requires things to use ApplicationTransactions, which is why this took so long to fix.
- The "attach stuff to the object" code has been in place for a long time and works correctly.
I'll land D8498 after this lands, too.
Test Plan:
- Uploaded via global homepage upload and file drag-and-drop upload, saw permissive visibility.
- Uploaded via comment area, saw restricted visibility.
- After commenting, verified links were established and the file became visible to users who could see the attached object.
- Verified Pholio (which is a bit of a special case) correctly attaches images.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10131
Summary: Ref T4589. Depends on D10129. In addition to letting users change the visibility policy for files, also allow them to choose a policy explicitly when a file is uploaded.
Test Plan: Uploaded several files using the plain old uploader, saw appropriate visibility policies applied.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10130
Summary: Ref T4589. Allow users to adjust visibility settings on files explicitly. This makes it easier to understand and manage upcoming changes in T4589.
Test Plan: Changed the view policy for a file several times.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10129
Summary: Ref T4896. This converts the last "CommentEditor" to a transaction editor and removes a large part of the old code.
Test Plan:
- Added comments.
- Accepted / added auditors.
- Added inline comments.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10128
Summary: Ref T4896. Invoke the new editor directly instead of in a roundabout way when handling Audit email.
Test Plan: Used `bin/mail receive-test` to simulate mail, saw comment post with proper content source.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10127
Summary: Ref T4896. Use the new transaction-oriented `PhabricatorAuditEditor` directly instead of invoking it via the old editor.
Test Plan: Used Conduit to add a comment, use silent mode, and accept a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10126
Summary: Ref T4896. Applies these actions using new transaction stuff.
Test Plan:
- Accepted and raised concern with my own commit, verifying the special project/package behavior.
- Accepted and raised concern with another author's commit, verifying the authority-over-packages/projects behavior.
- Accepted a commit I was not affiliated wiht, verifying the "join as an auditor" behavior.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10125
Summary: Ref T4896. Hook these up with new stuff.
Test Plan:
- Closed an audit.
- Resigned from an audit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10124
Summary:
Ref T4896. Move the write for "Add Auditors" inside the new Editor.
There are no longer any readers or writers for metadata, so remove the calls for it.
Test Plan: Added auditors from the web UI.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10123
Summary: Ref T4659. Because we JOIN, tasks with no value are filtered out. Instead, LEFT JOIN.
Test Plan: Issued an "Order by" and got all applicable tasks. Adjusted values and saw order change.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10119
Summary:
Resolves T4659. This implements support for sorting tasks by custom fields.
Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query.
Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10106
Summary: This class was renamed in D9991 but the filename is incorrect.
Test Plan: Eyeball it
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10118
Summary: Ref T4896. Instead of using custom stuff, use standard stuff.
Test Plan: Viewed a bunch of feed stories and published some over the Asana bridge.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10114
Summary: Ref T4896. Replace custom stuff with standard stuff.
Test Plan:
- Sent a bunch of email and it all looked sensible/correct.
- Made sure to test inlines, specifically, as they're a bit tricky.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10112
Summary: Ref T4896.
Test Plan: Made an unusual comment, then found it by searching.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10110
Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:
- No more fake proxy writes;
- no more fake detection of `@mentions`.
For now, the old code still applies most of the effects and handles feed and email.
Test Plan:
- Added comments.
- Added comments with inline comments.
- Added just inline comments.
- Added comments with Conduit.
- Previewed comments.
- Added CCs explicitly and with `@mentions`.
- Added auditors.
- Accepted a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10109
Summary:
Ref T4896. Currently, subscriptions to commits are stored as auditors with a special "CC" type.
Instead, use normal subscriptions storage, reads and writes.
Test Plan:
- Ran migration and verified data still looked good.
- Viewed commits in UI and saw "subscribers".
- Saw "Automatically Subscribed", clicked Subscribe/Unsubscribe on a non-authored commit, saw subscriptions update.
- Pushed a commit through Herald rules and saw them trigger subscriptions and auditors.
- Used "Add CCs".
- Added CCs with mentions.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10103
Summary: This class was renamed in D9991, but the filename is incorrect.
Test Plan: Eyeball it
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10117
Summary:
Fixes T5666. When we have a pretty link right now it can conflict with form data; e.g. if you have 'statuses=open' in the URI and then uncheck status = open in the UI, you will still get the open status in the next search.
To fix this, set the form action explicitly to lose all the get parameter junk.
Test Plan: tried the test case in T5666 / this description and it no longer failed...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5666
Differential Revision: https://secure.phabricator.com/D10115
Summary:
pre-patch "Mark All Read" marks *all* unread notifications as read. This is a race condition in that the user is looking at some set of notiifcations and that set may update such that the newest notifications aren't shown. An example might be if sitting on the notifications page or having the menu open while a new notification comes in... Note re-opening the menu would show the latest notifications.
This patch makes it so "Mark All Read" links only marks the notifications currently loaded (and older.) Fixes T5764.
Additionally, if there is nothing to "mark read" the button / link "Mark All Read" will have a disabled style and yield a dialog saying "nothing to mark as read".
Test Plan: carefully tracked ?chronoKey populating correctly in various links. Verified query constructed properly too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5764
Differential Revision: https://secure.phabricator.com/D10113
Summary: $this->id wasn't being set in this case so just set it explicitly after we finish loading the project. Fixes T5763.
Test Plan: links were broken no longer!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5763
Differential Revision: https://secure.phabricator.com/D10108
Summary: This moves the status property of the build to the bottom of the property list so that it matches the build targets.
Test Plan: Viewed a build, saw the status in the right position.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10096
Summary: This shows the status icon and color along side the build status on the build view controller.
Test Plan: Viewed a build, saw the icon appear.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10094
Summary:
Ref T1049. This uses tabs on build targets to hide the configuration details and variables by default, instead promoting the target name, it's status and a description of the build step. The description is a new field on each build step.
The primary advantage of having a description on build steps is that DevOps can configure appropriate description information (including any troubleshooting information for build failures) on build steps, and developers who have builds fail against their code review can then look at this information.
Test Plan: Viewed a build plan and saw the appropriate information.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10093
Summary: Ref T1049. This hides the build target messages area if there are no messages for the target. Since most of the time a build target won't recieve any messages, this area is confusing because it's always empty.
Test Plan: Viewed a build, saw the empty build target message areas disappear.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10092
Summary: We've received feedback that the "core - exception" is incredibly confusing, to the point where developers see this and write off the build failure as a Phabricator error that is unrelated to their changes.
Test Plan: Ran a build with a `exit 1` run step, didn't see the "core - exception" appear.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10090
Summary: This fix is wrong - should be load and not get - but moreover this is actually correctly set as the reply handler is instantiated inside the DifferentialRevisionMailReceiver correctly; $this->getExclude was correct. Ref T5185.
Test Plan: this shall stop the fatal in production.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10101
Summary: This makes input artifacts imply the appropriate build step dependencies in the build plan. That is, if you use a host artifact in a build step, it will then implicitly depend on the 'Lease Host' step.
Test Plan: Viewed the build plan with the artifacts, saw the dependencies. Ran a build, saw everything execute in the correct order.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10089
Summary: Ref T5185. By code inspection, I am pretty sure before this patch it was doing a set of a get on itself which does nothing. Now, being careful not to break Facebook we get the proper exclusion phids. I am pretty sure the folks in T5185 are experiencing this in Differential only.
Test Plan: Get some folks on T5185 to play with this
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10087
Summary:
Depends on D9806. This implements the build simulator, which is used to calculate the order of build steps in the plan editor. This includes a migration script to convert existing plans from sequential based to dependency based, and then drops the sequence column.
Because build plans are now dependency based, the grippable and re-order behaviour has been removed.
Test Plan: Tested the migration, saw the dependencies appear correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9847
Summary: Fixes T5751. Currently, we incorrectly overwrite `$xactions`.
Test Plan: Closed a blocking task, got an email about the correct transaction set ("closed task; added comment") instead of an overwritten transaction set ("closed blocking task").
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5751
Differential Revision: https://secure.phabricator.com/D10088
Summary: This was causing a crash according to a conversation with @rfergu in IRC. The issue is that if `$images_to_show` is empty, then `$image` never gets set, and it attempts to call `getID` on a null / unset variable.
Test Plan: @rfergu confirmed this fixed the crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, rfergu, epriestley
Differential Revision: https://secure.phabricator.com/D10073
Summary: Fixes T5653.
Test Plan: made a macro with sound and it played. changed it to not play and it did not play. changed it back and it played once more.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5653
Differential Revision: https://secure.phabricator.com/D10085
Summary:
This Fixes T5737. Apparently the functionality to search by different
statuses in differential was already there, but the options weren't
exposed in the frontend. I can't think of any reason why this should've
been the case, so I just added the other options.
Test Plan: Tested against some local diffs to match new query option.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5737
Differential Revision: https://secure.phabricator.com/D10076
Summary: At least on my install, sorting was pretty borked from a type issue. (e.g. "unbreak now" of 100 sorting as less than "High" of 90). Fix this with some parseInt action. Also support adding new cards with the new colsort stuff. The clever bit here is to include the task ID in the sorting vector because the task ID wins ties at the moment I think / new tasks need to show up before older tasks when they are initially created. Fixes T5716.
Test Plan: added many "normal" priority cards and saw them fly in correctly. changed priority and moved correctly. made no edits and no moves were made correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5716
Differential Revision: https://secure.phabricator.com/D10081
Summary: been some changes here and this code was broked. turns out we re-assign $action like two lines later and never used the initial value, so we can simply delete the offending line. Fixes T5745.
Test Plan: submitted inline comment pre-patch and fatal. re-submitted post patch and great success!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5745
Differential Revision: https://secure.phabricator.com/D10078
Summary: Currently, content runs before header. Instead, run header, content, footer. This makes it easier to use `header.php` for includes, for Phacility.
Test Plan: Made Phacility blog work better, see next diff.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10067
Summary:
Ref T4896. Depends on D10056. Moves search indexing to standard infrastructure.
Also, fixes a bug where inline comments would not be indexed.
Test Plan: Used `bin/search index ... --trace` to view index construction of a commit, saw all the comments and inlines get indexed.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10057
Summary: Ref T4896. Depends on D10055. This uses core rendering stuff for audit comments, and fixes all the wonkiness with inlines so we can actually land the migration.
Test Plan: Viewed, previewed and edited various types of comments in Diffusion.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10056
Summary:
Ref T4896. Depends on D10052. This is the major/scary migration, but not really so bad. It is substantially similar to D8210, but less complex because there are fewer actions here.
This moves `PhabricatorAuditComment` storage to `PhabricatorAuditTransaction`, then reads `PhabricatorAuditComment`s as a proxy around the new objects.
Test Plan:
- Before migrating, browsed around. Nothing appeared broken.
- Migrated cleanly.
- Viewed old transactions (inlines, comments, accept/reject/etc, add auditors, add ccs, implicit CCs).
- Added all of those comment types.
- Edited a draft.
- Deleted a draft.
- Spot checked the database for sanity.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10055
Summary:
Ref T4896. Depends on D10023. Prepares the code for the final migration.
The transaction table stores one row per distinct effect (e.g., add CCs) rather than one row per user action (e.g., "add CCs + comment"). We can double-read that table as long as the code doesn't expect transactions/comments to have multiple different effects, and doesn't try to write any such rows.
Everywhere that we were writing a big "X + Y" comment, write two separate "X" and "Y" comments instead. Like D10023, this disrupts the UI a little (you get more boxes), but that will be resolved once the rendering code swaps over. Otherwise, this retains the existing behavior.
Test Plan:
- Used `diffusion.createcomment` to add comments, raise concern, and accept.
- Previewed commenting, adding auditors/ccs, accepting, raising concern.
- Actually performed commenting, adding auditors/ccs, accepting, raising concern.
- Added a user with mentions.
- Added an explicit CC and a mention user.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10052
Summary:
Ref T4896. Moves us closer to migrating comments to transactions by building a transaction per inline.
This makes the UI a little wonky, and it will get slightly worse until we swap to the new UI and grouping/collapsing starts working. It's still usable, there's just a box per inline.
Test Plan:
- Added a comment.
- Added an inline comment.
- Added a comment and an inline comment.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10023
Summary: Adds Phriction to list of apps that use Source Sans as default font in addition to Legalpad and Diviner.
Test Plan: Tested various layouts imported from secure. Should be reasonably tested, but will follow up on secure.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10064
Summary:
Fixes T5707. After work on T5245, the `$task` object may not have the correct set of task PHIDs attached to it when Herald is invoked.
Instead, just fetch the authoritative set. This is simple, and Herald is not super performance sensitive (happens on a write) so hitting the DB once is fine.
Test Plan: Created a task with projects, verified the projects field contained the correct values when processed by Herald.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5707
Differential Revision: https://secure.phabricator.com/D10060
Summary: Fixes T5717. Like other partial edits, object links should not be blocked by unrelated missing fields on the object.
Test Plan:
- Linked two objects.
- Verified the inverse editor already sets "continue on missing fields" and "continue on no effect".
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5717
Differential Revision: https://secure.phabricator.com/D10059
Summary: Initially the change is aimed to solve issue with line breaks being lost in projects descriptions. But it is done in a general place so line breaks behavior is more consistent all over the place.
Test Plan:
- Write a multiline description of the project, using single \n for line breakers.
- View project details in project/view/X?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10014
Summary:
Handling readmes with no extension is a bit of a hack, but seemed like a small cost.
The Big Win here is that you can commit README.remarkup and README.md and have both Phabricator and GitHub render __with__ //all// ##the## ~~pretty~~ **markup**.
Test Plan: Looked at some readme files.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10047
Summary:
Instead of allowing all routes based on security.alternate-file-domain, now, when security.alternate-file-domain is set, and the request matches this domain, requests are validated against an explicit list. Allowed routes:
- /res/
- /file/data/
- /file/xform/
- /phame/r/
This will be redone by T5702 to be less of a hack.
Test Plan:
- browse around (incl. Phame live) to make sure there is no regression from this when security.alternate-file-domain is not used.
- check that celerity resources and files (incl. previews) are served with security.alternate-file-domain set.
- check that phame live blog is serving its css correctly with security.alternate-file-domain set.
- check that requests outside of the whitelist generate an exception for security.alternate-file-domain
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10048
Summary: As mentioned on rP8ce35e6b67e7e2a81b274bab7a6dd19dedb4df06, `setConcreteOnly(true)` can be omitted since (lacking magical powers) `loadObjects()` will always only instantiate concrete objects.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10050
Fixes an issue with D9991. A user was hitting the following exception:
```
echo '{}' | arc --conduit-uri='http://phabricator.joshuaspence.com'
call-conduit conduit.query
Waiting for JSON parameters on stdin...
Exception
[HTTP/500] Internal Server Error
>>> UNRECOVERABLE FATAL ERROR <<<
Call to a member function getAPIMethodName() on a non-object
/usr/src/phabricator/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php:34
┻━┻ ︵ ¯\_(ツ)_/¯ ︵ ┻━┻
(Run with --trace for a full exception trace.)
```
Auditors: epriestley
