Summary:
See T13193. See T13077. If we drop a column which is part of a UNIQUE KEY, MariaDB raises an error.
This is probably a bad idea on our side anyway, but in this case it wasn't an obviously bad idea.
To get around this:
- Drop the unique key, if it exists, before dropping the column.
- Explicitly add the new unique key afterward.
Test Plan: Ran `bin/storage upgrade` locally without issue, but I'm on MySQL. Will follow up on T13193.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19624
Summary:
See PHI844. Ref T13189.
Add "Revision test plan" as an available field for Herald. This is a little niche -- and a little odd because it sticks around even if you fully disable test plans -- but probably broadly reasonable.
The existing "Revision summary" field counterintuitively included the test plan. Separate this out since it's now a separate field and the behavior was weird historic nonsense. I'll note this in the changelog.
Test Plan: Wrote a rule using both fields, verified they generated the expected values.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19623
Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear.
Test Plan: {F5841997}
Reviewers: amckinley
Maniphest Tasks: T13077, T4815
Differential Revision: https://secure.phabricator.com/D19622
Summary:
Depends on D19620. Ref T13077. This adds a "Publish" operation which points the current version at some historical version of the document -- not necessarily the most recent version. Newer versions become "drafts".
This is still quite rough and missing a lot of hinting in the UI, I'm just making it work so I can start making the UI understand it.
Test Plan: Used the "Publish" action to publish older versions of a document, saw the document revert. Many UI hints are missing and this operation is puzzling and not yet usable for normal users.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19621
Summary: Depends on D19619. Ref T13065. Ref T13077. Migrate Phriction mail keys to the new infrastructure and drop the column.
Test Plan: Ran migrations, spot-checked the database.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13077, T13065
Differential Revision: https://secure.phabricator.com/D19620
Summary:
Ref T13077. This is mostly just a small cleanup change, even though the actual change is large.
We currently reference content and document objects from one another with `contentID` and `documentID`, but this means that `contentID` must be nullable. Switching to PHIDs allows the column to be non-nullable.
This also supports reorienting some current and future transactions around PHIDs, which is preferable for the API. In particular, I'm adding a "publish version X" transaction soon, and would rather callers pass a PHID than an ID or version number, since this will make the API more consistent and powerful.
Today, `contentID` gets used as a cheaty way to order documents by (content) edit time. Since PHIDs aren't orderable and stuff is going to become actually-revertible soon, replace this with an epoch timestamp.
Test Plan:
- Created, edited, moved, retitled, and deleted Phriction documents.
- Grepped for `documentID` and `contentID`.
- This probably breaks //something// but I'll be in this code for a bit and am likely to catch whatever breaks.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19619
Summary:
Ref T13077. We currently have these weird policy hints in Phriction that we don't use in other applications. Just remove them for consistency to make the eventual swap to EditEngine a little easier.
Also nuke some unreacahble code.
Test Plan: Loaded edit page, saw more standard UI.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19618
Summary:
Depends on D19616. Ref T13077. Fixes T8172. In the last round of design updates, a lot of actions got stuffed into "Actions" menus.
I never really got used to these and think they're a net usability loss, and broadly agree with the feedback in T8172. I'd generally like to move back toward a state where actions are available on the page, not hidden in a menu.
For now, just put a curtain view on these pages. This could be refined later (e.g., stick this menu to the right hand side of the screen) depending on where other Phriction changes go.
(Broadly, I'm also not satisfied with where we ended up on the fixed-width pages like Diffusion > Manage, Config, and Instances. In contrast, I //do// like where we ended up with Phortune in terms of overall design. I anticipate revisiting some of this stuff eventually.)
Test Plan:
- Looked at Phriction pages on desktop/tablet/mobile/printable -- actions are now available on the page.
- Looked at other DocumentView pages (like Phame blogs) -- no changes for now.
Reviewers: amckinley
Maniphest Tasks: T13077, T8172
Differential Revision: https://secure.phabricator.com/D19617
Summary: Ref T13077. There is no "PHUIDocumentView" so toss the "Pro" suffix from this classname.
Test Plan: Grepped for `PHUIDocumentView` and `PHUIDocumentViewPro`.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19616
Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc.
Test Plan: {F5840098}
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T13088, T9951
Differential Revision: https://secure.phabricator.com/D19615
Summary:
Fixes T12251. Ref T13189. See PHI610. The difficulty here is that we don't want to disclose Phabricator account information to Buildkite. We're comfortable disclosing information from `git`, etc.
- For commits, use the Identity to provide authorship information from Git.
- For revisions, use the local commit information on the Diff to provide the Git/Mercurial/etc author of the HEAD commit.
Test Plan:
- Built commits and revisions in Buildkite via Harbormaster.
- I can't actually figure out how to see author information on the Buildkite side, but the values look sane when dumped locally.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T12251
Differential Revision: https://secure.phabricator.com/D19614
Summary: Ref T13189. See PHI690. When a lease is first acquired or activated, note the time. This supports better visibility into queue lengths. For now, this is only queryable via DB and visible in the UI, but can be more broadly exposed in the future.
Test Plan: Landed a revision, saw the leases get sensible timestamps for acquisition/activation.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19613
Summary: Ref T13189. Summaries do not appear to be meaningfully rendered with Remarkup so just drop the engine. See D19610 for the previous change in this vein.
Test Plan: Viewed config list with option summaries.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19612
Summary:
Depends on D19609. Ref T13189. At some point, we switched from RemarkupEngine to RemarkupView and lost this piece of hack-magic.
Restore the hack-magic. It's still hack-magic instead of a real rule, but things are at least cleaner than they were before.
Test Plan: Viewed `auth.require-approval`, etc. Saw references to other config options linked properly.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19610
Summary: Ref T13189. When there are no setup issues, we currently double-render a weird setup issues box underneath the notice. Get rid of it.
Test Plan: Viewed page with and without setup issues, saw less awkward UI.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19609
Summary:
Depends on D19607. Ref T13189. See PHI642. Ref T13186.
Some transactions can sometimes be applied to objects you can not edit. Currently, using `*.edit` to edit an object always explicitly requires CAN_EDIT.
Now that individual transactions require CAN_EDIT by default and can reduce or replace this requirement, stop requiring CAN_EDIT to reach the editor.
The only expected effect of this change is that low-permission edits (like disabling a user, leaving a project, or leaving a thread) can now work via `*.edit`.
Test Plan:
- Tried to perform a normal edit (changing a task title) against an object with no CAN_EDIT. Still got a permissions error.
- As a non-admin, disabled other users while holding the "Can Disable Users" permission.
- As a non-admin, got a permissions error while trying to disable other users while not holding the "Can Disable Users" permission.
Reviewers: amckinley
Maniphest Tasks: T13189, T13186
Differential Revision: https://secure.phabricator.com/D19608
Summary:
Depends on D19606. Ref T13189. See PHI642.
- Disabling/enabling users no longer requires admin. Now, you just need "Can Disable Users".
- Update the UI to appropriately show the action in black or grey depending on what clicking the button will do.
- For "Approve/Disapprove", fix a couple bugs, then let them go through without respect for "Can Disable Users". This is conceptually a different action, even though it ultimately sets the "Disabled" flag.
Test Plan:
- Disabled/enabled users from the web UI as various users, including a non-administrator with "Can Disable Users".
- Hit permissions errors from the web UI as various users, including an administrator without "Can Disable Users".
- Saw the "Disable/Enable" action activate properly based on whether clicking the button would actually work.
- Disapproved a user without "Can Disable Users" permission, tried to re-disapprove a user.
- Approved a user, tried to reapprove a user.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19607
Summary:
Depends on D19605. Ref T13189. See PHI642. This adds a separate "Can Disable Users" capability, and makes the underlying transaction use it.
This doesn't actually let you weaken the permission, since all pathways need more permissions:
- `user.edit` needs CAN_EDIT.
- `user.disable/enable` need admin.
- Web UI workflow needs admin.
Upcoming changes will update these pathways.
Without additional changes, this does let you //strengthen// the permission.
This also fixes the inability to disable non-bot users via the web UI.
Test Plan:
- Set permission to "No One", tried to disable users. Got a tailored policy error.
- Set permission to "All Users", disabled/enabled a non-bot user.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19606
Summary:
Depends on D19604. Ref T13189. See PHI642. Deprecates these in favor of "user.edit", redefines them in terms of it, and removes the old `disableUser()` method.
I kept the "is admin" permissions check for consistency, since these methods have always said "(admin only)". This check may not be the most tailored check soon, but we can just keep executing it in addition to the real check.
For now, this change stops this method from actually disabling non-bot users (since it implicitly adds a CAN_EDIT requirement, and even administrators don't have that). An upcoming change will fix that.
Test Plan: Enabled and disabled a (bot) user via these methods. Checked API UI, saw them marked as "disabled".
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19605
Summary:
Ref T13189. See PHI642. Upgrades the "Disable" action in the web UI to be transaction-based.
This technically breaks things a little (you can't disable non-bot users, since they now require CAN_EDIT and you won't have it) but an upcoming change will fix the permissions issue.
Test Plan: Disabled and enabled a (bot) user from the web UI.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19604
Summary:
Depends on D19585. Ref T13164.
Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a `CAN_EDIT` check when loading the object to be edited.
A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are:
- Joining a project which you have CAN_JOIN on.
- Leaving a project which isn't locked.
- Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence).
- Leaving a Conpherence thread.
- Unsubscribing.
- Using the special `!history` command from email.
Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT:
- Adding comments.
- Subscribing.
- Awarding tokens.
Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users.
It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level.
Enforcement of these special cases is currently weird:
- We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers).
- To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor.
- Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes.
Instead, the new world order is:
- Every transaction has capability/policy requirements.
- The default is CAN_EDIT, but transactions can weaken this explicitly they want.
- So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake.
- And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT".
Test Plan:
- Tried to add members to a Conpherence thread I could not edit (permissions error).
- Left a Conpherence thread I could not edit (worked properly).
- Joined a thread I could see but could not edit (worked properly).
- Tried to join a thread I could not see (permissions error).
- Implemented `requireCapabilites()` on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error).
- Mentioned an object I can not edit on another object (works).
- Mentioned another object on an object I can not edit (works).
- Added a `{F...}` reference to an object I can not edit (works).
- Awarded tokens to an object I can not edit (works).
- Subscribed/unsubscribed from an object I can not edit (works).
- Muted/unmuted an object I can not edit (works).
- Tried to do other types of edits to an object I can not edit (correctly results in a permissions error).
- Joined and left a project I can not edit (works).
- Tried to edit and add members to a project I can not edit (permissions error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19586
Summary:
Ref T13187. See PHI811. If the file tree is enabled and visible, set the default tab to "History".
- This is a bit magic.
- It won't work entirely on mobile (we can't tell that you're on mobile on the server, so we'll pick the "History" tab even though the file tree isn't actually visible on your device).
- There's no corresponding logic in Diffusion. Diffusion doesn't have the same tab layout, but this makes things somewhat inconsistent.
So I don't love this, but we can try it and see if it's confusing or helpful on the balance.
Test Plan: With filetree on and off, reloaded revisions. Saw appropriate tab selected by default.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19601
Summary: Ref T13187. Ref T13176. With drafts, we actually want to suppress this link on "first broadcast" (the first time we send mail), not on "new object" (when the revision is created as a draft).
Test Plan: Poked at this locally, will keep an eye on it in production.
Reviewers: amckinley
Maniphest Tasks: T13187, T13176
Differential Revision: https://secure.phabricator.com/D19598
Summary: Ref T13187. See PHI807. The documentation currently does not make it very clear that this is a local setting, per `phd` process. Make it more clear.
Test Plan: {F5827757}
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19597
Summary:
Ref T13187. See PHI834. Mercurial has somewhat-recently (changeset is from Jan 2018) introduced a new "protocaps" command, that appears in Mercurial 4.7 and possibly before then.
We must explicitly enumerate all protocol commands because you can't decode the protocol without knowing how many arguments the command expects, so enumerate it.
(Also fix an issue where the related error message had an extra apostrophe.)
Test Plan:
- Ran `hg clone ...` with client and server on Mercurial 4.7.
- Before: fatal on unknown "protocaps" command.
- Midway: better typography in error message.
- After: clean clone.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19596
Summary:
Depends on D19594. See PHI823. Ref T13164.
- Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
- Add a label for the floating header display options menu in Differential.
- Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.
Test Plan:
Viewed a revision with `?__aural__=true`:
- Saw "Remove Action: ..." label.
- Saw "Display Options" label.
- Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19595
Summary: Ref T12164. Updates another controller to use identities.
Test Plan:
Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the `author` from this data structure is actually being used:
```
$return = array(
'commit' => $modified,
'date' => $date,
'author' => $author,
'details' => $details,
);
```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19580
Summary:
Ref T13164. See PHI823. (See that issue for some more details and discussion.)
Add aural labels to various buttons which were missing reasonable aural labels.
The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward.
Test Plan:
{F5806497}
{F5806498}
- Will follow up on the issue to make sure things are in better shape for the reporting user.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19594
Summary:
Depends on D19585. Ref T13164. This is a precursor for D19586, which causes Editors to start doing more explicit CAN_EDIT checks.
Passwords have an Editor, but don't actually define a CAN_EDIT capability. Define one (you can edit a password if you can edit the object the password is associated with).
(Today, this object is always a User -- this table just unified VCS passwords and Account passwords so they can be handled more consistently.)
Test Plan:
- With D19586, ran unit tests and got a pass.
- Edited my own password.
- Tried to edit another user's password and wasn't permitted to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19592
Summary: Depends on D19584. Ref T13164. This check is an //extra// check: you need EDIT //and// this capability. Thus, we can do it in validation without issues.
Test Plan:
- This code isn't reachable today: all methods of applying this transaction do a separate check for "Can Lock" upfront.
- Commented out the "Can Lock" check in the LockController, tried to lock as a user without permission. Was rejected with a policy exception.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19585
Summary:
Depends on D19583. Ref T13164. This continues the work of getting rid of `requireCapabilities()`.
This check is valid, but can be a `validateTransactions()` check instead. This is generally more consistent with how other applications work (e.g., creating subprojects).
The UI for this isn't terribly great: you get a policy error //after// you try to create the object. But that's how it worked before, so this isn't any worse than it was. The actual policy exception is (very) slightly more clear now (raised against the right object).
Test Plan:
- Created a child as a user with permission to do so to make sure I didn't break that.
- Set edit permission on `a/` to just me, tried to create `a/b/` as another user, got a policy exception since they can't edit the parent.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19584
Summary: Depends on D19582. Ref T13164. It's not possible to reach the editor without passing through a CAN_EDIT check, and it shouldn't be necessarily to manually specify that edits require CAN_EDIT by default.
Test Plan: Grepped for `RepositoryEditor`, verified that all callsites pass through a CAN_EDIT check.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19583
Summary:
Depends on D19581. Ref T13164. This method has no effect:
- You must always have CAN_EDIT to reach an Editor in the first place.
- Per previous change, I'm going to restructure this so transactions explicitly check CAN_EDIT by default anyway.
Test Plan: Tried to edit and hide a project column as a user without permission, hit global permission checks long before reaching this method.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19582
Summary:
Depends on D19579. Fixes T10003. These have been deprecated with a setup warning about their impending removal for about two and a half years.
Ref T13164. See PHI642. My overall goal here is to simplify how we handle transactions which have special policy behaviors. In particular, I'm hoping to replace `ApplicationTransactionEditor->requireCapabilities()` with a new, more clear policy check.
A problem with `requireCapabilities()` is that it doesn't actually enforce any policies in almost all cases: the default is "nothing", not CAN_EDIT. So it ends up looking like it's the right place to specialize policy checks, but it usually isn't.
For "Disable", I need to be able to weaken the check selectively (you can disable users if you have the permission, even if you can't edit them otherwise). We have a handful of other edits which work like this (notably, leaving and joining projects) but they're very rare.
Test Plan: Grepped for all removed classes. Edited a Maniphest task.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164, T10003
Differential Revision: https://secure.phabricator.com/D19581
Summary:
Depends on D19577. Ref T13164. See PHI642. This adds modern transaction-oriented enable/disable support.
Currently, this also doesn't let you disable normal users even when you're an administrator. I'll refine the policy model later in this change series, since that's also the goal here (let users set "Can Disable Users" to some more broad set of users than "Administrators").
This also leaves us with two different edit pathways: the old UserEditor one and the new UserTransactionEditor one. The next couple diffs will redefine the other pathways in terms of this pathway.
Test Plan:
- Enabled/disabled a bot.
- Tried to disable another non-bot user. This isn't allowed yet, since even as an administrator you don't have CAN_EDIT on them and currently need it: right now, there's no way for a particular set of transactions to say they can move forward with reduced permissions.
- Tried to enable/disable myself. This isn't allowed since you can't enable/disable yourself.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19579
Summary:
Depends on D19576. Ref T13164. See PHI642. This adds an EditEngine for users and a `user.edit` modern API method.
For now, all it supports is editing real name, blurb, title, and icon (same as "Edit Profile" from the UI).
Test Plan:
- Edited my stuff via the new API method.
- Tried to edit another user, got rejected by policies.
- Tried to create a user, got rejected by policies.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19577
Summary:
Ref T13164. See PHI642. I'd like to provide a third-generation `user.edit` API endpoint and make `user.enable` and `user.disable` obsolete before meddling with policy details, even if it isn't full-fledged yet.
Users do already have a transactions table and a Transaction-based editor, but it's only used for editing title, real name, etc. All of these are custom fields, so their support comes in automatically through CustomField extension code.
Realign it for modular transactions so new code will be fully modern. There are no actual standalone transaction types yet so this diff is pretty thin.
Test Plan:
- Grepped for `UserProfileEditor`.
- Edited a user's title/real name/icon.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19576
Summary: Depends on D19491.
Test Plan: Viewed some commits where the identity was mapped to a user and another that wasn't; saw the header render either a link to the user or the identity object.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19492
Summary:
Ref T13164. See PHI788. The issue requests a "created" timestamp.
Also add filtering for repository, state, and author.
Test Plan:
Used all filters.
{F5795085}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19574
Summary:
Fixes T13184. In Almanac, interfaces are always added to devices. However, if you "Add New Interface" and then "Cancel", you go to the nonexistent `/interface/` page.
Instead, return to the device page.
Test Plan: From a device page, clicked "Add Interface" and then "Cancel". Ended up back where I was.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13184
Differential Revision: https://secure.phabricator.com/D19573
Summary:
Ref T13164. See PHI785. See D19546. I think I didn't test the updated error messaging here entirely properly, since I have some tasks in queue which error out here ("Missing argument 1 to newMailers(...)").
This is an error condition already, but we want to get through this call so we can raise a tailored message.
Test Plan: Tasks which errored out here now succeed. This condition is only reachable if you misconfigure things in the first place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19572
Summary:
Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files.
We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI.
Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled.
Test Plan:
- Viewed a revision in normal and standalone views.
- No changes in normal view, and all keys still work ("N", "P", etc).
- In standalone view, "?" no longer shows nonfunctional key commands.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19571
Summary: Ref T12164. Defines a new manual activity that suggests rebuilding repository identities before Phabricator begins to rely on them.
Test Plan:
- Ran migration, observed expected setup issue: {F5788217}
- Ran `bin/config done identities` and observed setup issue get marked as done.
- Ran `/bin/storage upgrade --apply phabricator:20170912.ferret.01.activity.php` to make sure I didn't break the reindex migration; observed reindex setup issue appear as expected.
- Ran `./bin/config done reindex` and observed reindex issue cleared as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19497
Summary:
Fixes T12397. Ref T13164. See PHI801.
Several installs have hit various use cases where the path on disk where Phabricator lives changes at runtime. Currently, `bin/ssh-auth` caches a flat file which includes the path to `bin/ssh-exec`, so this may fall out of date if `phabricator/` moves.
These use cases have varying strengths of legitimacy, but "we're migrating to a new set of hosts and the pool is half old machines and half new machines" seems reasonably compelling and not a problem entirely of one's own making.
Test Plan:
- Compared output on `master` to output after change, found them byte-for-byte identical.
- Moved `phabricator/` to `phabricator2/`, ran `bin/ssh-auth`, got updated output.
- Added a new SSH key, saw it appear in the output.
- Grepped for `AUTHFILE_CACHEKEY` (no hits).
- Dropped the cache, verified that the file regenerates cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164, T12397
Differential Revision: https://secure.phabricator.com/D19568
Summary:
Ref T13164. See PHI790. Older versions of APCu reported cache keys as "key" from `apcu_cache_info()`. APC and newer APCu report it as "info".
Check both indexes for compatibility.
Test Plan:
- Locally, with newer APCu, saw no behavioral change.
- Will double check on `admin`, which has an older APCu with the "key" behavior.
- (I hunted this down by dumping `apcu_cache_info()` on `admin` to see what was going on.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19569
Summary:
Ref T13164. In PHI801, an install reported a particular slow Conduit method call.
Conduit calls aren't easily profilable with normal tools (for example, `arc call-conduit --xprofile ...` gives you a profile of the //client//). They can be profiled most easily with `bin/conduit call ... --xprofile`.
However, `bin/conduit call` currently doesn't let you pick a user to execute the command on behalf of, so it's not terribly useful for profiling `*.edit`-style methods which do a write: these need a real acting user.
Test Plan:
Ran `bin/conduit call --method user.whoami --as epriestley ...` with valid, invalid, and no acting users.
```
$ echo '{}' | ./bin/conduit call --method user.whoami --as epriestley --input -
Reading input from stdin...
{
"result": {
"phid": "PHID-USER-icyixzkx3f4ttv67avbn",
"userName": "epriestley",
"realName": "Evan Priestley",
...
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19566
Summary:
Ref T13164. See PHI766. Currently, when file data is stored in small chunks, we submit each chunk to the indexing engine.
However, chunks are never surfaced directly and can never be found via any search/query, so this work is pointless. Just skip it.
(It would be nice to do this a little more formally on `IndexableInterface` or similar as `isThisAnIndexableObject()`, but we'd have to add like a million empty "yes, index this always" methods to do that, and it seems unlikely that we'll end up with too many other objects like these.)
Test Plan:
- Ran `bin/harbormaster rebuild-log --id ... --force` before and after change, saw about 200 fewer queries after the change.
- Uploaded a uniquely named file and searched for it to make sure I didn't break that.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19563
Summary:
Ref T13164. See PHI725. For real "*.search" methods, parameters get validated and you get an error if you use an empty list as a constraint.
Since "transaction.search" isn't really a normal "*.search" method, it doesn't benefit from this. Just do the check manually for now.
Test Plan: Made `transaction.search` calls with no constraints (got results); a valid costraint (got fewer results); and an invalid empty constraint (got an exception).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19562
Summary:
Ref T13164. See PHI797. The last edit is available in the page header, but it's not precise (just says "180 days ago") and a little weird (it's unusual for us to put that kind of information in the header).
Add a precise timestamp to the footer for now. I'd imagine re-examining this the next time Phriction gets some UI work and maybe trying to integrate timeline/transactions more cleanly (see also T1894).
Test Plan: Looked at a wiki page, then edited it. Saw precise "Last Edit" timestamp adjacent to "Last Author".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19560
Summary:
See D19558. This method has no callers and just wraps `diffusion.historyquery`, since D5960 (2013).
This was introduced in D315 (which didn't make it out of FB, I think) inside Facebook for unclear purposes in 2011.
Test Plan: Grepped for callers, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: artms
Differential Revision: https://secure.phabricator.com/D19559
Summary:
`diffusion.getrecentcommitsbypath` fails with 500 error when non existing callsign is passed:
```
>>> UNRECOVERABLE FATAL ERROR <<<
Call to a member function getCommit() on null
```
Expected Behavior:
Return more graceful error notifying caller that such callsign/repository does not exist
Reproduction steps:
Open conduit: https://secure.phabricator.com/conduit/method/diffusion.getrecentcommitsbypath/
Enter:
callsign: "obviouslynotexisting"
path: "/random"
Click call method
Test Plan: after applying patch - call no longer fails with 500s
Reviewers: Pawka, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19558
Summary:
Depends on D19556. See PHI765. Ref T13164. Currently, if you type `H1` in this datasource, it isn't smart enough to pull up the right object.
Add support for querying by monogram. This is similar to existing support in Owners packages, etc.
Test Plan: Typed `H1` in the new push log filter, got the right object as a result in the typeahead.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19557
Summary: Depends on D19555. Ref T13164. See PHI765. An install is interested in getting a sense of the impact of a particular blocking rule, which seems reasonable. Support filtering for pushes blocked by a particular rule or set of rules.
Test Plan: {F5776385}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19556
Summary:
Ref T13164. See PHI765. We currently show "Rejected: Herald" in the push log UI, but don't show which rule rejected a push.
We store this data, and it's potentially useful: either for hunting down a particular issue, or for getting a general sense of how often a reject rule is triggering (maybe because you want to tune how aggressive it is).
Show this data in the web UI, and include it in the data export payload.
Test Plan:
- Pushed to a hosted repository so that I got blocked by a Herald rule.
- Viewed the push logs in the web UI, now saw which rule triggered things.
- Exported logs to CSV, saw Herald rule PHIDs in the data.
{F5776211}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19555
Summary:
Depends on D19550. Ref T13164. See T12144#226172, mostly. We get some requests to make milestones reorderable, but in most cases users probably wanted subprojects, not milestones.
One reason to end up here is that we put "Milestones" on top. Instead, put "Subprojects" on top, since they're the less specialized option and we aren't terribly consistent about it anyway.
Test Plan: Viewed project subprojects page, saw "Subprojects" above "Milestones".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19551
Summary: Depends on D19552. Ref T13164. We need this little `setObject(...)` hook to get the Space name into the search list UI.
Test Plan: Viewed project list, saw some Spaces listed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19554
Summary:
Ref T13164. See PHI774. Fixes T12435.
Since Phriction is hierarchical, there isn't a super strong motivation to support Spaces: you can generally set policies on a small number of documents to get the desired effective policy behavior.
However, it still improves consistency and there's no reason //not// to support Spaces. In the case where you have some moderately weird/complex policy on one or more Spaces, using Spaces to define the policy behavior can make things a bit simpler and easier to understand.
This probably doesn't actually fix whatever the root problem in T12435 was (complicated, non-hierarchical access policies?). See also a bunch of discussion in T12442. So we might end up going beyond this to address other use cases, but I think this is reasonable regardless.
Test Plan: Created and edited Phriction documents and shifted them between Spaces. Searched by Space, etc.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13164, T12435
Differential Revision: https://secure.phabricator.com/D19553
Summary:
Depends on D19549. Ref T13164. See PHI774.
- Make milestones inherit their parent project's space automatically, like they inherit their parent policies.
- Make subprojects default to their parent project's space.
Test Plan: Created subprojects and milestones, got sensible default/effective Space behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19550
Summary:
See PHI774. Ref T13164. There is no reason projects //don't// support Spaces, just a vague concern that it's not hugely useful and might be a bit confusing.
However, it's at least somewhat useful (to improve consistency and reduce special casing) and doesn't necessarily seem more confusing than Projects are anyway. Support is trivial from a technical point of view, so just hook it up.
Test Plan: Created new projects, shifted projects between spaces. The support is all pretty much automatic.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19549
Summary:
See PHI766. Ref T13164. Build log chunk processing does a `preg_split()` on slices, but this isn't terribly efficient.
We can get the same count more cheaply by just using `substr_count()` a few times.
(I also tried `preg_match_all()`, which was between the two in speed.)
Test Plan:
- Used `bin/harbormaster rebuild-log --id X --force` to rebuild logs. Verified that the linemap is identical before/after this change.
- Saw local time for the 18MB log in PHI766 drop from ~1.7s to ~900ms, and `preg_split()` drop out of the profiler (we're now spending the biggest chunk of time on `gzdeflate()`).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19545
Summary:
See PHI785. Ref T13164. In this case, an install wants to receive mail via Mailgun, but not configure it (DKIM + SPF) for outbound mail.
Allow individual mailers to be marked as not supporting inbound or outbound mail.
Test Plan:
- Added and ran unit tests.
- Went through some mail pathways locally, but I don't have every inbound/outbound configured so this isn't totally conclusive.
- Hit `bin/mail send-test` with a no-outbound mailer.
- I'll hold this until after the release cut so it can soak on `secure` for a bit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19546
Summary:
Ref T13151. See PHI616. Fixes T8163.
This adds `/D123/new/`, which shows the changes to the revision since the last timeline action you took.
It also adds a link to this view to diff update emails.
Test Plan:
- Followed this link with a recent comment and no touches since update, ended up with sensible diff selections.
- Updated revision, generated email, saw an appropriate link.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151, T8163
Differential Revision: https://secure.phabricator.com/D19541
Summary:
See PHI751. Ref T13164. We added a "silent" flag for Editors somewhat recently (currently reachable only for bulk edits with `bin/bulk ...` command).
However, this flag doesn't carry through to the sub-editor when we make inverse edge edits. These are edits like "X is a parent of Y", which cause an implicit "Y is a child of X" edit to occur.
Pass the flag through.
Test Plan:
- Rigged the relationships controller to make silent edits.
- Changed the parents of a revision from the web UI. Saw no mail or feed stories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19543
Summary:
See PHI749. Ref T13164. We currently misdetect files starting with `[submodule ...` as JSON.
Make this a bit stricter:
- If the file is short, just see if it's actually literally real JSON.
- If the file is long, give up.
This should get the right result in pretty much all the cases people care about, I think. We could make the long-file guesser better some day.
Test Plan: Detected a `[submodule ...` file (no longer JSON) and a `{"duck": "quack"}` file (still JSON).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19544
Summary:
Fixes T13172. At one point we always capitalized all the text, and the cache uses capitalized text.
However, we stopped capitalizing the text at some point. Modern memes are more more subtle than old memes, and when we eventualy add support for things like "explodey brain" we'll certainly want to support mixed case.
Practically, this stops you from changing the capitalization of a cached meme. Get rid of the cache transform.
Test Plan:
none lul
(I don't have `gd` installed locally and buiding it requires building libjpeg and libpng or giving up and using `brew`. I'l vet this in production.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13172
Differential Revision: https://secure.phabricator.com/D19537
Summary:
See T7148. This just cheats us out of a weird sort of race where we:
- Dump an instance, including some `F123` which is a temporary file which expires in 3 minutes.
- A few minutes later, the daemons delete the data for that file.
- A few minutes after that, we try to `bin/files migrate --copy` to copy the data from S3 into the MySQL blob store.
- This fails since the data is already gone.
Instead, just skip these files since they're already dead to us.
Test Plan: Faked this locally, will migrate the PHI769 instance on `aux001`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19536
Summary:
When generating test data to solve a bug I have encountered, I noticed Lipsum was not working correctly for Differential Revisions and Pastes.
It seemed like they weren't updated after some refactoring. This fixes that by updating them.
Test Plan: Run Lipsum for all objects, and note that it has much less failure.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D19534
Summary:
Depends on D19529. See PHI778.
- Document the "name" constraint as deprecated. All callers are likely better served by the "query" constraint.
- Guide users toward the "query" constraint a little better.
- Document the `=` syntax.
Test Plan: Read various new documentation.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19531
Summary:
Ref PHI778. In D18492, I added support for parsing this operator, but did not actually implement it in the query engine.
Implementation is fairly straightforward. This supports querying for objects by exact title with `title:="exact title"`. This is probably a bad idea, but sometimes maybe useful anyway.
Test Plan: Queried for `title:="xxx"`, found only exact matches.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: ahoffer2
Differential Revision: https://secure.phabricator.com/D19529
Summary: Fixes T13171. Open to suggestions but that face looks real, real dumb on High Sierra.
Test Plan: Visited Config, saw a serious professional emoji in the page title.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13171
Differential Revision: https://secure.phabricator.com/D19530
Summary:
See PHI775. See D19499. Originally, see PHI720.
D19499 broke the standalone "Branches" page for commits. Normally, you reach this by taking these steps:
- View a commit which is contained by 11 or more branches.
- Click the "More Branches..." link in the "Branches" field.
- You should be taken to a list of all branches which contain the commit.
The change to the 'branch' parameter was adjusted in the query that builds the "x, y, z, More Branches..." list, but not on the actual "Branches" list with the full list. Adjust it.
Test Plan:
- Set display limit to 1, viewed a commit on "master" and "stable", clicked "More Branches".
- Before: saw only "master".
- After: saw both "master" and "stable".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19532
Summary: Ref T13168. This is just a small quality-of-life fix: we can disclose which public key we're talking about because public keys are public.
Test Plan:
- Hit public key error (through my own bumbling / not reading or following instructions). Specifically, I haven't associated the key with a device in Almanac.
- Before: vague error.
- After: more specific error with enough key material that I could grep for it.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13168
Differential Revision: https://secure.phabricator.com/D19516
Summary:
See PHI746. See also T11833, perhaps. Ref T13151.
Long ago, parent revisions were called "dependent revisions". This was changed to "parent revisions" in the action UI to improve clarity, but not changed in the timeline stories.
Update the timeline stories to use the same language the actions in the UI use.
Test Plan:
{F5732876}
{F5732877}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19514
Summary:
Ref T13151. See PHI727. Update the dashboard widget/panel datasource to actually query results using what the user typed.
The current approach is blind to what the user typed when pulling results from the database, and gets limited to an artificially small number of results somewhere in the pipeline.
Test Plan:
- Queried for panels with text queries.
- Queried for panels with `W123` queries.
- This is substantially similar to the Owners datasource, which received a similar update in D17142 and has worked well since then.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19511
Summary:
See PHI725. Ref T13151. These actions are somewhat unusual and I considered different ways to represent them (make them look like "status" transactions; build multiple synthetic transactions) but ultimately landed on the simplest approach of just exposing them more or less as they exist internally.
I haven't included data for any of them. Most don't really have any data, but "accept" does. I'm holding off on providing more data until after T731, which may shake up the internal format.
Test Plan: Applied most of these transactions against a revision, queried for it with `transaction.search`, got distinguishable transactions out.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19509
Summary: See PHI725. Ref T13151. We currently try to load comments unconditionally, but not all objects (like projects) have comments. Only try to load comments if an object actually has comments.
Test Plan: Queried for an object with no comments, like project `#masonry`, via `transaction.search`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19507
Summary:
Ref T13151. See PHI725. By default, "transaction.search" doesn't provide details about transactions because many have bad/weird/policy-violating internal types or fields.
The "create" transaction is simple and straightforward, so label it to allow callers to distinguish it.
Test Plan:
- Created a new task.
- Called `transaction.search` on it.
- Saw the labelled "create" transaction.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: swisspol
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19505
Summary:
Depends on D19503. Ref T13151. See PHI719. If you have something like a script which updates an object in a loop, we can end up queueing many search reindex tasks.
These tasks may reasonably contend for the lock, especially if the object is larger (lots of text and/or lots of comments) and indexing takes a few seconds.
This isn't concerning, and the indexers should converge to good behavior quickly once the updates stop.
Today, they'll spew a bunch of serious-looking lock exceptions into the log. Instead, just yield so it's more clear that there's (normally) no cause for concern here.
Test Plan: Ran `bin/search index Txxx --force` on a large object in multiple windows with a 0 second lock, saw an explicit yield instead of a lock exception.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19504
Summary:
Depends on D19502. Ref T13151. See PHI719. An install ended up with an object with 111,000+ comments on it because someone wrote a script to treat it like a logfile.
Although we seem to do mostly okay with this (locally, it only takes about 30s to index a similar object) we'll hit a wall somewhere (since we need to hold everything in memory), and it's hard to imagine a legitimate object with more than 1,000 comments. Just ignore comments past the first thousand.
(Conpherence threads may legitimately have more than 1,000 comments, but go through a different indexer.)
Test Plan:
- Piped some comments into `maniphest.edit` in a loop to create a task with 100K comments.
- Ran `bin/search index Txxx --force` to reindex it, with `--trace`.
- Before: task indexed in about 30s.
- After: script loaded comments with LIMIT 1000 and indexed in a couple seconds.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19503
Summary:
Ref T13151. See PHI719. One minor hiccup in debugging the issue (which ended up being "revision has 100K comments") was that the `SearchWorker` did not show which object it was indexing.
Add `'objectPHID'` to the queue call so you can see which object is affected from the web UI.
Test Plan:
- Stopped daemons.
- Used `bin/search index D123 --background` to queue a search task.
- Viewed task details in web UI from `/daemon/`.
- Before change: no indication of which object was being indexed.
- After change: page helpfully shows that the task is indexing D123.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19502
Summary:
Fixes T13159. Two issues here:
- When viewing a particular config setting, there's an extra "Config" crumb.
- On the page for a config group, the link to the parent group has an extra "/config/" in it.
Test Plan:
- Viewed a page for a particular setting, no longer saw an extra "Config" crumb.
- Viewed a page for a setting group, clicked parent crumb, got taken to a real page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13159
Differential Revision: https://secure.phabricator.com/D19501
Summary:
Ref T13151. See PHI720. If you want to test if commit X appears on specific branch Y, `git branch --contains X -- Y` is faster than (effectively) `git branch --contains X | grep Y`.
Since this call has a "branch" parameter anyway, use it as the pattern argument if provided.
Test Plan:
- Called the API method with no parameters, got all branches.
- Called the API method with `master`, got just master.
- Called the API method with `maste*`, got master. This behavior is not officially supported and may change in the future.
- Viewed a commit, still saw all branches.
- Grepped for `diffusion.branchquery` and verified that no remaining callsites pass a default "branch" parameter.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19499
Summary: Fixes T13155. Ref T13151. A recent change (D19455) changed the return format here, but I missed this special case for empty commits.
Test Plan:
- T13155 has a good set of reproduction instructions.
- Pushed an empty commit.
- Before: bunch of warning log spew.
- After: clean logs.
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T13155, T13151
Differential Revision: https://secure.phabricator.com/D19500
Summary: I landed D19491 a little aggressively, so allow this field to be null until after the migration goes out.
Test Plan: Loaded commits without identity objects; did not get any errors.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19496
Summary: Ref T12164. Make it easier to work with identity objects by attaching them to commits and attaching users to identities.
Test Plan: Loaded some commits with `->needIdentities(true)` and checked the resulting objects.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19491
Summary: This never worked.
Test Plan: Ran `bin/repository rebuild-identities` and viewed identity objects with `currentEffectiveUserID`s and no longer got errors about attempting to attach `null` objects instead of `PhabricatorUser` objects.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19495
Summary:
See <https://discourse.phabricator-community.org/t/commit-6011085b0fcd-breaks-sending-certain-email/1571>. Some mailers get upset if we `setHTMLBody(...)` with an empty string.
There's some possible argument they should be more graceful about this, but it's reasonably pretty ambiguous.
Only try to set the HTML body if we actually have a nonempty HTML body.
Test Plan:
- Configured an "smtp" mailer.
- Ran `echo hi | ./bin/mail send-test --to someone@somewhere.com --subject test`.
- Before: error about empty message body.
- After: no more message body error.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19494
Summary:
Ref T13151. Ref T12164. Two small tweaks:
- If we aren't actually going to change anything, just skip the writes. This makes re-running/resuming a lot faster (~20x, locally).
- Print when we touch a commit so there's some kind of visible status.
This is just a small quality-of-life tweak that I wrote anyway while investigating T13152, and will make finishing off db024, db025 and db010 manually a little easier.
Test Plan:
- Set `authorIdentityPHID` + `committerIdentityPHID` to `NULL`.
- Ran `rebuild-identities`, saw status information.
- Ran `rebuild-identiites` again, saw it go faster with status information.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151, T12164
Differential Revision: https://secure.phabricator.com/D19484
Summary:
Ref T13151. See T11767. See PHI686. Although we limit outbound mail text bodies, the limit doesn't currently apply to attachments, HTML bodies, or headers. T11767 discusses improving this in the general case.
In the wild, an install hit an issue (see PHI686) where edits to Phriction pages generate very large HTML bodies. Check and respect the limit when building HTML bodies.
If we don't have enough room for the HTML body, we just drop it. We have the text body to fall back to, and HTML is difficult to truncate safely.
Test Plan: Added unit tests and made them pass.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19489
Summary:
Ref T13151. See PHI683. Ref T12314.
You can currently change object subtypes via Conduit (`maniphest.edit`) but not via the web UI.
Changing object subtypes is inherently a somewhat-perilous operation that likely has a lot of rough edges we'll need to smooth over eventually, mostly around changing an object from subtype X to subtype Y, where some field exists on one but not the other. This isn't a huge issue, just not entirely intuitive.
It should also, in theory, be fairly rare.
As a reasonable middle ground, provide web UI access via the bulk editor. This makes it possible, but doesn't clutter the UI up with a rarely-used option with rough edges.
Test Plan:
- With subtypes not configured, saw a normal bulk editor with no new option.
- With subtypes configured, swapped tasks subtypes via bulk editor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151, T12314
Differential Revision: https://secure.phabricator.com/D19490
Summary:
Depends on D19487. Ref T13151. See PHI647. For some objects, like revisions, we can build slightly more useful secure email without actually disclosing anything.
In the general case, the object monogram may disclose information (`#acquire-competitor`) but most do not, so applications can whitelist an acceptable nondisclosing subject and link.
Support doing this, and make Differential do it. When we don't have a whitelisted URI but do know the object the mail is about, include a generic PHID-based URI; these are always nondisclosing.
Test Plan:
- Without the Differential changes, sent normal mail (no changes) and secure mail (new generic PHID-based link).
- With the Differential changes, sent secure mail; got richer subject and body link.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19488
Summary:
Ref T13151. See PHI647. This allows us to link to any object by PHID, without disclosing information in the monogram (like `#fire-steve`).
This capability is relevant when building "secure mail", to provide a link to the object regardless of whether the monogram discloses information or not.
Test Plan: Visited `/object/D123/` (redirect), `/object/xyz/` (404), `/object/PHID-DREV-.../` (redirect).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19487
Summary:
Ref T13151. See PHI702. An install is interested in a "members of all projects" (vs "members of any project", which is currently implemented) rule.
Although this is fairly niche, I think it's reasonable and doesn't have much of a maintenance cost.
This could already be implemented as an extension, but it would have to copy/paste a bunch of code.
Test Plan:
- Ran unit tests.
- Used the UI to select this policy for a task, with various values. Joined/left projects to satisfy/fail the rule. Behavior seemed correct.
- Used the UI to select the existing policy rule ("any project"), joined/left projects to satisfy/fail the rule. Doesn't look like I broke anything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19486
Summary:
Ref T13151. See PHI701. Unified diffs are currently missing the logic to apply the "old-full" and "new-full" classes, which results in a too-light coloration for fully added or removed lines.
Make this logic consistent with the two-up renderer so we use the same colors in both.
Test Plan: Viewed diffs and swapped between 1-up and 2-up renderers, now saw the same coloration.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19482
Summary:
Ref T13151. See PHI654. Depends on D19477. If you have long package names, the table of contents (e.g., in Differential) can end up expanding to be gigantic.
Getting tables to behave nicely is hard (or, at least, I can't figure it out after spending a decent amount of time on it; see also `AphrontTableView::renderSingleDisplayLine()`). I tried a bunch of things and Googled for a bit but didn't make any progress on finding a CSS solution. Just truncate the package names to get reasonable behavior without falling down any kind of CSS rabbit hole.
Test Plan:
- Created a package named "Very long package name...".
- Created a package named "MMMMMMMMMMMMMMMMMMMMMM...".
- Had them own a file in a Differential revision, viewed that revision.
- Before: table is pushed out to several times the browser window width and everything is kind of a mess.
- After: package names get truncated to something reasonable.
{F5652953}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19478
Summary:
Ref T13151. See PHI684. Currently, the `MailableFunction` datasource does not include Owners packages, but they are valid subscribers and the `Mailable` datasource includes them.
Include them in the `MailableFunction` datasource, too.
Test Plan: Searched for revisions with particular package subscribers, got expected results in the UI (tokenizer knew about packages) and response.
Reviewers: amckinley, jmeador
Reviewed By: jmeador
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19476
Summary:
Fixes T13145. The list controllers properly support public access already, but some of the view/detail controllers did not.
Allow logged-out users to browse builds, buildables, plans, etc., provided they can see the corresponding objects.
Test Plan: As a logged-out user, browsed around builds, build plans, logs, etc., without hitting any login pages.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13145
Differential Revision: https://secure.phabricator.com/D19459
Summary:
See PHI689. It can be difficult to distinguish between cards with the same number but different expiration dates (common when the bank sends you a new card).
For now, show the expiration date on the cart checkout screen.
Test Plan: Viewed a cart checkout screen with multiple cards, saw expiration dates.
Reviewers: amckinley
Differential Revision: https://secure.phabricator.com/D19462
Summary:
Depends on D19443. Creates a workflow for populating the new identity table by iterating over commits, either one repo at a time or all at once. Locally caches identities to avoid fetching them `inf` times. An actual migration that invokes this workflow will come in another revision that won't land until at least next week.
Performance is ~2k commits in 4.9s on my local machine.
Test Plan: Ran locally a few times with a few different states of the `repository_identity` table.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: jcox, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19446
Summary: Depends on D19429. Depends on D19423. Ref T12164. This creates new columns `authorIdentityPHID` and `committerIdentityPHID` on commit objects and starts populating them. Also adds the ability to explicitly set an Identity's assignee to "unassigned()" to null out an incorrect auto-assign. Adds more search functionality to identities. Also creates a daemon task for handling users adding new email address and attempts to associate unclaimed identities.
Test Plan: Imported some repos, watched new columns get populated. Added a new email address for a previous commit, saw daemon job run and assign the identity to the new user. Searched for identities in various and sundry ways.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19443
Summary: Depends on D19423. Ref T12164. Adds controllers capable of listing and editing `PhabricatorRepositoryIdentity` objects. Starts creating those objects when commits are parsed.
Test Plan: Reparsed some revisions, observed objects getting created in the database. Altered some `Identity` objects using the controllers and observed effects in the database. No attempts made to validate behavior under "challenging" author/committer strings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19429
Summary: Ref T12164. Start building initial objects for managing `RepositoryIdentity` objects. This won't land until much more of the infrastructure is in place.
Test Plan: Ran `bin/storage upgrade` and observed expected table.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19423
Summary:
Ref T13142. When commits are pushed, we try to handle them on one of two pathways:
- Normal changes: we load these into memory and potentially apply Herald content rules to them.
- "Enormous" changes: we don't load these into memory and skip content rules for them.
The goal is to degrade gracefully when users push huge changes: they should work, just not support all the features.
However, some changes can slip through the cracks right now:
- If you push a lot of commits at once, we'll try to cache all of the changes smaller than 1GB in memory. This can require an arbitrarily large amount of RAM.
- We calculate sizes by just looking at the `strlen()` of the diff, but a changeset takes more RAM in PHP than the raw diff does. So even if a diff is "only" 500MB, it can take much more memory than that. On systems with relatively little memory available, this may result in OOM while processing changes that are close to the "enormous" limit.
This change makes two improvements:
- Instead of caching everything, cache only 64MB of things.
- For most pushes, this is the same, since they have less than 64MB of diffs.
- For pushes of single very large changes, this is a bit slower (more CPU) since we have to do some work twice.
- For pushes of many changes, this is slower (more CPU) since we have to do some work twice, but, critically, doesn't require unlimited memory.
- Instead of flagging changes as "enormous" at 1GB, flag them as "enormous" at 256MB.
- This reduces how much memory is required to process the largest "non-enormous" changes.
- This also gets us under Git's hard-coded 512MB "always binary" cutoff; see T13143.
- This is still completely gigantic and way larger than any normal change should be.
An additional improvement would be to try to reduce the amount of memory we need to use to hold a change in process memory. I think the other changes here alone will fix the immediate issue in PHI657, but it would be nice if the "largest non-enormous change" required only a couple gigs of RAM.
Test Plan:
- Used `ini_set('memory_limit', '1G')` to artificially limit memory to 1GB.
- Pushed a series of two commits which add two 550MB text files (Temporarily, I added a `--binary` flag to trick Git into showing real diffs for these, see T13143.)
- Got a memory limit error.
- Applied the "cache only 64MB of stuff" and "consider 256MB, not 1GB, to be enormous" changes.
- Pushed again, got properly rejected as enormous.
- Added `memory_get_usage()` calls to measure how actual memory size and reported "size" estimate compare. For these changes, saw a 639MB diff require 31,479MB of memory, i.e. a factor of about 50x. This is, uh, pretty not great.
- Allowed enormous changes, pushed again, push went through.
Reviewers: amckinley
Maniphest Tasks: T13142
Differential Revision: https://secure.phabricator.com/D19455
Summary:
Ref T13137. See that task for discussion.
When we show a diff-of-diffs, we often render stubs for files which didn't change between the diffs. These stubs usually aren't a big deal, but for certain types of changes (like refactors) they can create a lot of clutter.
Instead, hide these stubs and show a notice that we hid them.
Test Plan:
- Created a revision affecting 4 files.
- Updated it with a diff that changed only 1 of the 4 files.
- Added an inline comment to a different file.
- Viewed the diff of diffs.
- Before: 4 changesets with two "nothing changed" stubs.
- After: 2 changesets with the stubs hidden.
{F5621083}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19453
Summary:
Ref T13137. The "analyze/cache data about changesets" step is becoming more involved. We recently added detection for generated code to support "Ignore generated changes" in Owners, and I now plan to hash the new file content so we can hide changes which have no effect.
Before adding this new hashing step, pull the "detect copied code" and "detect generated code" stuff out and move them to a separate `ChangesetEngine`. Then support doing a changeset rebuild directly with `bin/differential rebuild-changesets`.
This simplifies things a bit and makes testing easier since you don't need to keep creating new revisions to re-run copy/generated/hash logic.
Test Plan: Ran `bin/differential rebuild-changesets --revision Dxxx`, saw changesets rebuild. See also next change.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19452
Summary:
Fixes T13140. See PHI660.
Recent versions of Subversion can send a `(get-file true false false )` protocol frame with extra space between "false" and "false". This is allowed by the protocol spec, but never normally happens, and we do not parse it correctly.
Instead, parse it correctly.
Test Plan:
- Added unit tests.
- Ran `svn proplist svn+ssh://.../diffusion/X/file.c` under SVN 1.10 before and after the change.
- Before: indefinite hang.
- After: completed in finite time.
Reviewers: amckinley, asherkin
Reviewed By: amckinley, asherkin
Maniphest Tasks: T13140
Differential Revision: https://secure.phabricator.com/D19451
Summary: Ref PHI662. Viewing a dashboard you don't have permission to view (in the Dashboard application) currently fatals while building crumbs, since we fail to build the ` ... > Dashboard 123 > ...` crumb.
Test Plan:
- Viewed a dashboard I didn't have permission to view in the Dashboards application.
- Before patch, fatal when calling `getID()` on a non-object.
- After patch, sensible policy error page.
- Viewed a dashboard I can view, saw sensible crumbs.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19449
Summary:
See <https://hackerone.com/reports/351361>. We currently require MFA on the screen leading into the user create flow, but not the actual create flow.
That is, `/people/create/` (which is just a "choose a type of account" page) requires MFA, but `/people/new/<type>/` does not, even though this is the actual creation page.
Requiring MFA to create users isn't especially critical: creating users isn't really a dangerous action. The major threat is probably just that an attacker can extend their access to an install by creating an account which they have credentials for.
It also isn't consistently enforced: you can invite users or approve users without an MFA check.
So there's an argument for just removing the check. However, I think the check is probably reasonable and that we'd likely prefer to add some more checks eventually (e.g., require MFA to approve or invite) since these actions are rare and could represent useful tools for an attacker even if they are not especially dangerous on their own. This is also the only way to create bot or mailing list accounts, so this check does //something// on its own, at least.
Test Plan:
- Visited `/people/new/standard/` as an admin with MFA configured.
- Before patch: no MFA prompt.
- After patch: MFA prompt.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19448
Summary: Ref T13137. See PHI592. Depends on D19444. Apply a limit up front to stop patches which are way too big (e.g., 600MB of videos) from generating in the first place.
Test Plan:
- Configured inline patches in git format.
- Created a normal revision, got an inline git patch.
- Created a revision with a 10MB video file, got no inline patch.
- (Added a bunch of debugging stuff to make sure the internal pathway was working.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19445
Summary:
See <https://discourse.phabricator-community.org/t/maniphest-home-page-crash-after-d19417/1445/3>. These special-token-only searches currently end up populating an empty `ownerPHIDs`, which fatals after the stricter check in D19417.
Make the fatal on `withConstraint(array())` explicit and only set the PHID constraint if we have some PHIDs left.
Test Plan: Searched for "No Owner", "Any Owner", an actual owner, "No Owner + actual user".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19440
Summary:
See PHI652. When you `echo x | arc paste` today, you end up with a Paste object that has the empty string as its "language".
This is normally not valid. Pastes where the language should be autodetected should have the value `null`, not the empty string.
This behavior likely changed when `paste.create` got rewritten in terms of `paste.edit`. Adjust the implementation so it only adds the LANGUAGE transaction if there's an actual language.
Also, fix an issue where you can't use the "delete" key to delete tokens with the empty string as their value.
Test Plan:
- Created a paste with `echo x | arc paste`, got a paste in autodetect mode instead of with a bogus language value.
- Created a paste with `echo x | arc paste --lang rainbow`, got a rainbow paste.
- Deleted an empty string token with the keyboard.
- Deleted normal tokens with the keyboard.
- Edited subscribers/etc normally with the keyboard and mouse to make sure I didn't ruin anything.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19437
Summary:
Ref T13137. See PHI609. An install would like to filter audit requests on a particular branch, e.g. "master".
This is difficult in the general case because we can not apply this constraint efficiently under every conceivable data shape, but we can do a reasonable job in most practical cases.
See T13137#238822 for more detailed discussion on the approach here.
This is a bit rough, but should do the job for now.
Test Plan:
- Filtered commits by various branches, e.g. "master"; "lfs". Saw correct-seeming results.
- Stubbed out the "just list everything" path to hit the `diffusion.internal.ancestors` path, saw the same correct-seeming results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19431
Summary:
See <https://discourse.phabricator-community.org/t/maniphest-non-integer-point-values-in-csv-export/1443>.
We currently export the Maniphest "points" field as an integer, but allow it to accept decimal values (e.g. "6.25").
Also fix a bug where we wouldn't roll over from "..., X, Y, Z, AA, AB, ..." correctly for Excel column names if sheet had more than 26 columns.
Test Plan:
- Set a task point value to 6.25.
- Exported to text, JSON, XLS.
- Saw 6.25 represented accurately in exports.
- Exported an excel sheet with 27+ columns.
- Manually printed the first 200 column names to check that the algorithm looks correct.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19434
Summary:
See PHI251. Ref T13137.
- Replace the perplexing text box with a checkbox that explains what it does.
- Mention this feature in the documentation.
Test Plan:
- Clicked/unclicked checkbox.
- Read documentation.
- Used an existing checkbox control in Slowvote to make sure I didn't break it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19433
Summary:
See <https://discourse.phabricator-community.org/t/hidden-description-field-in-maniphest-task-breaks-form/1432>.
If you hide the "Description" field in Maniphest, we still try to render a remarkup preview for it. This causes a JS error and a nonfunctional element on the page.
Instead, hide the preview panel if the field has been locked or hidden.
Test Plan:
- Hid the field, loaded the form, no more preview panel / JS error.
- Used a normal form with the field visible, saw a normal preview.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19432
Summary:
See PHI638. When a diff is large (between 100 and 1000 files), we collapse content by default unless a change also has inline comments.
This rule isn't explicitly explained anywhere. Although it's not really a critical rule, it fits easily enough into the UI callout.
Also render the UI callout in a slightly more modern way and avoid `hsprintf()`.
Test Plan:
{F5596496}
- Also, clicked the "Expand" link and saw everything expand properly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19430
Summary:
Depends on D19427. Ref T13130. See PHI251. Support configuring owners packages so they ignore generated paths.
This is still a little rough. A couple limitations:
- It's hard to figure out how to use this control if you don't know what it's for, but we don't currently have a "CheckboxesEditField". I may add that soon.
- The attribute ignore list doesn't apply to Diffusion, only Differential, which isn't obvious. I'll either try to make it work in Diffusion or note this somewhere.
- No documentation yet (which could mitigate the other two issues a bit).
But the actual behavior seems to work fine.
Test Plan:
- Set a package to ignore paths with the "generated" attribute. Saw the package stop matching generated paths in Differential.
- Removed the attribute from the ignore list.
- Tried to set invalid attributes, got sensible errors.
- Queried a package with Conduit, got the ignored attribute list.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19428
Summary:
Depends on D19426. Ref T13130. Ref T13065. While I'm making changes to Owners for "Ignore generated paths", clean up the "mailKey" column.
We recently (D19399) added code to automatically generate and manage mail keys so we don't need a ton of `mailKey` properties in the future. Migrate existing mail keys and blow away the explicit column on packages.
Test Plan: Ran migration, manually looked at the database and saw sensible data. Edited a package to send some mail, which looked good.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13130, T13065
Differential Revision: https://secure.phabricator.com/D19427
Summary:
Depends on D19425. Ref T13130. See PHI251. Now that changesets have a durable "generated" attribute, we can let owners packages check it when we're computing which packages are affected by a revision.
There's no way to actualy configure a package to have this behavior yet.
Test Plan:
- Created a revision affecting a generated file and a non-generated file.
- When I faked `mustMatchUngeneratedPaths()` to `return true;`, saw the non-generated file get no packages owning it.
- Normally: lots of packages owning it).
- Created a revision affecting only generated files.
- When I faked things, saw no Owners actions trigger.
- Normally: some packages added reviewers or subscribers.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19426
Summary:
Ref T13130. See PHI251. Currently, changesets are marked as "generated" (i.e., the file contains generated code and does not normally need to be reviewed) at display time.
An install would like support for having Owners rules ignore generated files. Additionally, future changes anticipate making "generated" and some other similar behaviors more flexible and more general.
To support these, move toward a world where:
- Changesets have "attributes": today, generated. In the future, perhaps: third-party, highlight-as, encoding, enormous-text-file, etc.
- Attributes are either "trusted" (usually: the server assigned the attribute) or "untrusted" (usually: the client assigned the attribute). For attributes like "highlight-as", this isn't relevant, but I'd like to provide tools so that you can't make `arc` mark every file as "generated" and sneak past review rules in the future.
Here, the `differential.generated-paths` config can mark a file as "generated" with a trusted attribute. The `@generated`-in-content rule can mark a file as "generated" with an untrusted attribute.
Putting these attributes on changesets at creation time instead of display time will let Owners interact with changesets cheaply: it won't have to render an entire changeset just to figure out if it's generated or not.
Test Plan:
- Created a revision touching several files, some generated and some not.
- Saw the generated files get marked properly with attribute metadata in the database, and show/fold as "Generated" in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19425
Summary:
Fixes T13135. See PHI633. For at least some video files with legitimate MIME type "video/quicktime", Chrome can play them but refuses to if the `<source />` tag has a `type="video/quicktime"` attribute.
To trick Chrome into giving these videos the old college try, omit the "type" attribute. Chrome then tries to play the video, seems to realize it can, and we're back on track.
Since the "type" attribute is theoretically only useful to help browsers select among multiple different alternatives and we're only presenting one alternative, this seems likely safe and reasonable. Omitting "type" also validates. It's hard to be certain that this won't cause any collateral damage, but intuitively it seems like it should be safe and I wasn't able to identify any problems.
Test Plan:
- Watched a "video/quicktime" MP4 cat video in Chrome/Safari/Firefox.
- See T13135 for discussion, context, and discussion of the behavior of some smaller reproduction cases.
Reviewers: amckinley, asherkin
Reviewed By: amckinley
Maniphest Tasks: T13135
Differential Revision: https://secure.phabricator.com/D19424
Summary:
Fixes T13132. I removed this branch in D19156 when tightening the logic for the new CSP header, but there's a legitimate need for it: downloading files via `arc download`, or more generally being an API consumer of files.
This is not completely safe, but attacks I'm aware of (particularly, cookie fixation, where an attacker could potentially force a victim to become logged in to an account they control) are difficult and not very powerful. We already issue clear setup advice about the importance of configuring this option ("Phabricator is currently configured to serve user uploads directly from the same domain as other content. This is a security risk.") and I think there's significant value in letting API clients just GET file data without having to jump through a lot of weird hoops.
Test Plan:
- With `security.alternate-file-domain` off, tried to `arc download` a file.
- Before: downloaded an HTML dialog page.
- After: downloaded the file.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13132
Differential Revision: https://secure.phabricator.com/D19421
Summary:
Ref T13130. See <https://discourse.phabricator-community.org/t/unable-to-create-owners-package-with-same-path-in-multiple-repositories/1400/1>.
When you edit paths in Owners, we deduplicate similar paths, like `/x/y` and `/x/y/`. However, this logic currently only examines the paths, and incorrectly deduplicates the same path in different repositories.
Instead, consider the repository before deduplicating.
Test Plan:
- Edited an Owners package and added the path "/" in two different repositories.
- Before: only one surived the edit.
- After: both survived.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19420
Summary: See PHI611 for details.
Test Plan:
Ran a Buildkite build, saw Buildkite confirm receipt of these parameters in the HTTP response:
{F5562054}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19419
Summary:
Depends on D19416. Ref T13110. Ref T13130. See PHI598. When rendering a "Very Large" revision (affecting more than 1,000 files) we currently compute the package/changeset ownership map normally.
This is basically a big list of which packages own which of the files affected by the change. We use it to:
# Show which packages own each file in the table of contents.
# Show an "(Owns No Changed Paths)" hint in the reviewers list to help catch out-of-date packages that are no longer relevant.
However, this is expensive to build. We don't render the table of contents at all, so (1) is pointless. The value of (2) is very small on these types of changes, and certainly not worth spending many many seconds computing ownership.
Instead, just skip building out these relationships for very large changes.
Test Plan: Viewed a very large change with package owners; verified it no longer built package map data and rendered the package owners with no "(Owns No Changed Paths)" hints.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13110
Differential Revision: https://secure.phabricator.com/D19418
Summary:
Ref T13110. Ref T13130. When a revision is "large" (100 - 1000 files) we hide the actual textual changes by default. When it is "very large" (more than 1000 files) we hide all the changesets by default.
For "very large" diffs, we currently still show the "large" warning, which doesn't really make sense since there aren't any actual changesets.
When a diff is "very large", don't show the "large" warning.
Test Plan:
- Viewed a small diff (<100 files), saw no warnings.
- Viewed a large diff (100-1000 files), saw just the large warning.
- Viewed a very large diff (>1000 files).
- Before: both "large" and "very large" help warnings.
- After: just "very large" warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13110
Differential Revision: https://secure.phabricator.com/D19416
Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.
Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.
Test Plan:
- Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
- Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
- Viewed stuff in Files (no blame UI options).
- Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13105
Differential Revision: https://secure.phabricator.com/D19414
Summary: See discussion in D19415.
Test Plan: Searched for some owners, found tasks as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19417
Summary:
See PHI285. Ref T13130. After recent changes Herald sends email about rules, but the mail doesn't currently actually include a link to the rule.
Include a link for consistency and ease-of-use.
Test Plan: Edited a rule, looked at the resulting mail, saw a link to the rule.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19413
Summary:
Fixes T13128. Ref PHI590. This is a rough-and-ready implementation of a new `PhabricatorPolicyCodex->compareToDefaultPolicy()` method that subclasses can override to handle special cases of policy defaults. Also implements a `PolicyCodex` for Phriction documents, because the default policy of a Phriction document is the policy of the root document.
I might break this change into two parts, one of which maintains the current behavior and another which implements `PhrictionDocumentPolicyCodex`.
Test Plan: Created some Phriction docs, fiddled with policies, observed expected colors in the header. Will test more comprehensively after review for basic reasonable-ness.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, swisspol
Maniphest Tasks: T13128
Differential Revision: https://secure.phabricator.com/D19409
Summary:
See PHI615. Ref T13130. An install is reporting that "Lease Working Copy" build steps always report "Built instantly" after completion.
I'm not 100% sure that this is the fix, but I'm like 99% sure: "Lease Working Copy" build steps yield after they ask Drydock for a lease. They will later reenter `doWork()`, see that the lease is filled, and complete.
Right now, we reset the start time every time we enter `doWork()`. Instead, set it only if it hasn't been set yet.
Test Plan: This is low-risk and a bit tricky to reproduce locally, but I'll run some production builds and see what they look like.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19412
Summary:
Ref T13130. See PHI619. Currently, the Herald "Test Console" doesn't pass a "Content Source" to the adapter, so if any rules of the given type execute a "Content source" field rule, they'll fatal.
Provide a content source:
- If possible, use the content source from the most recent transaction.
- Otherwise, build a default "web" content source from the current request.
Test Plan:
- Wrote a "When [content source][is][whatever]" rule for tasks.
- Ran test console against a task.
- Before: got a fatal trying to interact with the content source.
- After: transcript reports sensible content source.
- Also commented out the "xaction" logic to test the fallback behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19411
Summary:
Ref T13103. Locally, I managed to break the data for a bunch of files by doing `git clean -df` in a working copy that I'd updated to a commit from many many years ago. Since `conf/local.json` wasn't on the gitignore list many years ago, this removed it, and I lost my encryption keyring.
I've symlinked my local config to a version-controlled file now to avoid this specific type of creative self-sabotage in the future, but this has exposed a few cases where we could handle things more gracefully.
One issue is that if your favicon is customized but the file it points at can't actually be loaded, we fail explosively and you really can't do anything to move forward except somehow guess that you need to fix your favicon. Instead, recover more gracefully.
Test Plan:
- Configure file encryption.
- Configure a favicon.
- Remove the encryption key from your keyring.
- Purge Phabricator's caches.
- Before: you pretty much dead-end on a fatal that's hard to understand/fix.
- After: everything works except your favicon.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13103
Differential Revision: https://secure.phabricator.com/D19406
Summary:
See <https://twitter.com/HayleyCAnderson/status/988873585363009536>.
Currently, the action dropdown in Differential shows a heavy "X" after "Request Changes" and a heavy checkmark after "Accept Revision".
Although I'm not convinced that the messaging around "Request Changes" is too strong, I do think these marks are out of place in modern Differential. They came from a simpler time when this dropdown had fewer actions, but feel a little weird and inconsistent to me in the modern UI.
Let's try getting rid of them and see how it goes?
Test Plan:
- Viewed these actions in the dropdown, no longer saw the mark icons.
- Grepped for these unicode sequences without getting any other hits.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19405
Summary:
Depends on D19400. Ref T13130. Currently, when you write Herald rules about other Herald rules, you can't pick a rule type or content type, so there's no way to get notified about edits to just global rules (which is the primary driving use case).
Add a "Content type" field to let the rule match rules that affect revisions, tasks, commits, etc.
Add a "Rule type" field to let the rule match global, personal, or object rules.
Test Plan:
- Wrote a global rule for other rules about global Herald rules:
{F5540307}
{F5540308}
- Ran it against itself which matched:
{F5540309}
- Ran it against another rule (not a global rule about Herald rules), which did not match:
{F5540311}
- Also reviewed the fields in those transcripts in more detail to make sure they were extracting matching correctly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19403
Summary:
Depends on D19399. Ref T13130. This adds basic support for writing Herald rules against Herald rules. See T13130 for a lot more detail.
This needs a bit more work to be useful: for example, there's no way to specify the rule type or subject, so you can't say "notify me when global rules are edited" or "notify me when Maniphest rules are edited". I'll add some fields for that in followup changes to actually solve the original use case.
Test Plan:
- Wrote Herald rules against Herald rules.
- Ran them by editing rules and in the test console.
- Verified they sent some mail with `bin/mail list-outbound`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19400
Summary:
Ref T13065. `mailKey`s are a private secret for each object. In some mail configurations, they help us ensure that inbound mail is authentic: when we send you mail, the "Reply-To" is "T123+456+abcdef".
- The `T123` is the object you're actually replying to.
- The `456` is your user ID.
- The `abcdef` is a hash of your user account with the `mailKey`.
Knowing this hash effectively proves that Phabricator has sent you mail about the object before, i.e. that you legitimately control the account you're sending from. Without this, anyone could send mail to any object "From" someone else, and have comments post under their username.
To generate this hash, we need a stable secret per object. (We can't use properties like the PHID because the secret has to be legitimately secret.)
Today, we store these in `mailKey` properties on the actual objects, and manually generate them. This results in tons and tons and tons of copies of this same ~10 lines of code.
Instead, just store them in the Mail application and generate them on demand. This change also anticipates possibly adding flags like "must encrypt" and "original subject", which are other "durable metadata about mail transmission" properties we may have use cases for eventually.
Test Plan:
- See next change for additional testing and context.
- Sent mail about Herald rules (next change); saw mail keys generate cleanly.
- Destroyed a Herald rule with a mail key, saw the mail properties get nuked.
- Grepped for `getMailKey()` and converted all callsites I could which aren't the copy/pasted boilerplate present in 50 places.
- Used `bin/mail receive-test --to T123` to test normal mail receipt of older-style objects and make sure that wasn't broken.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D19399
Summary:
Ref T13130. See PHI483. Currently, "Plan Changes + Draft" uses rules like "Plan Changes", not rules like "Draft", and allows "Accept".
This isn't consistent with how "Draft" and "Accept" work in other cases. Make "Plan Changes + Draft" more like "Draft" for consistency.
Also fix a string that didn't have a natural English version.
Test Plan:
- Added a failing build plan.
- Created a revision.
- Loaded the revision before builds completed, saw a nicer piece of text about "waiting for builds" instead of "waiting for 2 build(s)".
- Builds failed, which automatically demoted the reivsion to "Changes Planned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense (particularly, no "Accept").
- Abandoned the revision to test "Abandoned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense.
- Reclaimed the revision, then used "Request Review" to send it to "Needs Review". Verified that actions made sense and, e.g., reviewers could now "Accept" normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19398
Summary: Depends on D19391. Ref T13126. See that task for some details on what's going on here.
Test Plan:
- Viewed a file which includes lines that were added during the first commit to the repository.
- Before D19391: fatal.
- After D19391: blank.
- After this patch: accurate blame information.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13126
Differential Revision: https://secure.phabricator.com/D19392
