Summary: Show SSH user on git-over-ssh repositories and hide both username and password for other repos.
Test Plan: View repository details page in diffusion, Clone URI should appear with a username (taken from repo config) and any http(s) repos should be without usernames.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4147
Differential Revision: https://secure.phabricator.com/D7631
Summary: Simplifies the code a bit and fixes all the wonky previews. Fixes T4053
Test Plan: Test all pages, logged in and logged out.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4053
Differential Revision: https://secure.phabricator.com/D7622
Summary: D7590 made path construction more consistent, but affected this callsite if a subpath is configured. Currently, we end up with double `@@` in the URI.
Test Plan:
- Ran unit tests.
- Ran `bin/repostitory discover`.
Reviewers: staticshock, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7619
Summary:
Ref T4140. We could hit a redirect loop for a user with a verified primary email address but no "is verified" flag on their account. This shouldn't be possible since the migration should have set the flag, but we can deal with it more gracefully when it does happen (maybe because users forgot to run `storage/upgrade`, or because of ghosts).
In the controller, check the same flag we check before forcing the user to the controller.
When verifying, allow the verification if either the email or user flag isn't set.
Test Plan: Hit `/login/mustverify/`; verified an address.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4140
Differential Revision: https://secure.phabricator.com/D7621
Summary: Ref T4140. Allow unapproved users to verify their email addresses. Currently, unapproved blocks email verification, but should not.
Test Plan: Clicked email verification link as an unapproved user, got email verified.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4140
Differential Revision: https://secure.phabricator.com/D7618
Summary: ...and get the basic edit flow "working" for a new NuanceSourceDefinition - the Phabricator Form. ...and fix a dumb bug in the query class so when you redirect to the view page / try to edit an existing NuanceSource you don't fatal.
Test Plan: played around with the edit form and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7585
Summary:
Fixes T4041. We currently detect when "origin" is incorrect, but can do better:
- When "origin" is missing, we can add it. This happens for Git 1.7.1 -- see T4041.
- When "origin" is wrong, we can fix it automatically if we control the repository.
We only need to fail when origin exists, is wrong, and we aren't in charge of the repository.
Test Plan: Ran `bin/repository discover X` on a repository with a good origin, no origin, a bad-but-under-control origin, and a bad-out-of-control origin. Got the right behavior in all cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, champo
Maniphest Tasks: T4041
Differential Revision: https://secure.phabricator.com/D7614
Summary:
This cleans up some garbage:
- We were specifying environmental variables with `X=y git ...`, but now have `setEnv()` on both `ExecFuture` and `PhutilExecPassthru`. Use `setEnv()`.
- We were specifying the working directory with `(cd %s && git ...)`, but now have `setCWD()` on both `ExecFuture` and `PhutilExecPassthru`. Use `setCWD()`.
- We were specifying the Git credentials with `ssh-agent -c (ssh-add ... && git ...)`. We can do this more cleanly with `GIT_SSH`. Use `GIT_SSH`.
- Since we have to write a script for `GIT_SSH` anyway, use the same script for Subversion and Mercurial.
This fixes two specific issues:
- Previously, we were not able to set `-o StrictHostKeyChecking=no` on Git commands, so the first time you cloned a git repo the daemons would generally prompt you to add `github.com` or whatever to `known_hosts`. Since this was non-interactive, things would mysteriously hang, in effect. With `GIT_SSH`, we can specify the flag, reducing the number of ways things can go wrong.
- This adds `LANG=C`, which probably (?) forces the language to English for all commands. Apparently you need to install special language packs or something, so I don't know that this actually works, but at least two users with non-English languages have claimed it does (see <https://github.com/facebook/arcanist/pull/114> for a similar issue in Arcanist).
At some point in the future I might want to combine the Arcanist code for command execution with the Phabricator code for command execution (they share some stuff like LANG and HGPLAIN). However, credential management is kind of messy, so I'm adopting a "wait and see" approach for now. I expect to split this at least somewhat in the future, for Drydock/Automerge if nothing else.
Also I'm not sure if we use the passthru stuff at all anymore, I may just be able to delete that. I'll check in a future diff.
Test Plan: Browsed and pulled Git, Subversion and Mercurial repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7600
Summary:
Ref T2230. SVN has some weird rules about path construction. Particularly, if you're missing a "/" in the remote URI right now, the change parsing step doesn't build the right paths.
Instead, build the right paths more intelligently.
Test Plan: Added and executed unit tests. Imported an SVN repo.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, jpeffer
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7590
Summary:
Fixes T4132. If you run "bin/auth recover" before setting the base URI, it throws when trying to generate a production URI.
Instead, just show the path. We can't figure out the domain, and I think this is less confusing than showing "your.phabricator.example.com", etc.
Test Plan: Ran `bin/auth recover <user>` for valid and missing base-uri.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4132
Differential Revision: https://secure.phabricator.com/D7615
Summary:
Ref T4122. Implements a credential management application for the uses described in T4122.
@chad, this needs an icon, HA HA HAHA HA BWW HA HA HA
bwahaha
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7608
Summary:
We've been having trouble with viewing diffs timing out when there's a lot of unit test failures. It was caused by formatting userdata for every single failure. The expensive part of this was actually creating the engine for every result, so moved the construction outside of the loop.
Diffs that timed out (2 min) loading before load in around 6 seconds now.
Test Plan: Loaded diffs that used to time out. Verified that details still looked right when Show Full Unit Test Results Is Clicked.
Reviewers: epriestley, keegancsmith, lifeihuang, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, andrewjcg
Differential Revision: https://secure.phabricator.com/D7581
Summary:
Ref T4110. This denormalized field used to power "Group By: Assigned" got dropped in the T2217 migration at some point.
Restore its population, and fix all the data in the database.
Test Plan: Ran migration, verified database came out reasonable-looking. Reassigned a task, verified database. Ran a "Group By: assigned" query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4110
Differential Revision: https://secure.phabricator.com/D7602
Summary:
Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.
Fix this in two ways:
# Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
# In this case, load the revision for an omnipotent viewer.
This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.
Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4109
Differential Revision: https://secure.phabricator.com/D7603
Summary:
Ref T2230. When fully set up, we have up to three users who all need to write into the repositories:
- The webserver needs to write for HTTP receives.
- The SSH user needs to write for SSH receives.
- The daemons need to write for "git fetch", "git clone", etc.
These three users don't need to be different, but in practice they are often not likely to all be the same user. If for no other reason, making them all the same user requires you to "git clone httpd@host.com", and installs are likely to prefer "git clone git@host.com".
Using three different users also allows better privilege separation. Particularly, the daemon user can be the //only// user with write access to the repositories. The webserver and SSH user can accomplish their writes through `sudo`, with a whitelisted set of commands. This means that even if you compromise the `ssh` user, you need to find a way to escallate from there to the daemon user in order to, e.g., write arbitrary stuff into the repository or bypass commit hooks.
This lays some of the groundwork for a highly-separated configuration where the SSH and HTTP users have the fewest privileges possible and use `sudo` to interact with repositories. Some future work which might make sense:
- Make `bin/phd` respect this (require start as the right user, or as root and drop privileges, if this configuration is set).
- Execute all `git/hg/svn` commands via sudo?
Users aren't expected to configure this yet so I haven't written any documentation.
Test Plan:
Added an SSH user ("dweller") and gave it sudo by adding this to `/etc/sudoers`:
dweller ALL=(epriestley) SETENV: NOPASSWD: /usr/bin/git-upload-pack, /usr/bin/git-receive-pack
Then I ran git pushes and pulls over SSH via "dweller@localhost". They successfully interacted with the repository on disk as the "epriestley" user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7589
Summary: Although I don't want to end up with 20 of these again, this is a reasonable default to provide, particularly for installs where a large portion of the userbase primarily reports bugs and does not interact with them directly.
Test Plan: Hit `/maniphest/`, saw "Subscribed", clicked it, saw the tasks I'm subscribed to.
Reviewers: jbrown, btrahan
Reviewed By: jbrown
CC: aran
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D7586
Summary:
A usable, Land to GitHub flow.
Still to do:
- Refactor all git/hg stratagies to a sane structure.
- Make the dialogs Workflow + explain why it's disabled.
- Show button and request Link Account if GH is enabled, but user is not linked.
- After refreshing token, user ends up in the settings stage.
Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.
Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.
Test Plan:
Went through these flows:
- load revision in hosted, github-backed, non-github backed repos to see button as needed.
- hit land with weak token - sent to refresh it with the extra scope.
- Land to repo I'm not allowed - got proper error message.
- Successfully landed; Failed to apply patch.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7555
Summary:
Ref T4039. This fixes an issue where a user with the ability to create repositories could view repositories he is otherwise not permitted to see, by following these steps:
- Suppose you want to see repository "A".
- Create a repository with the same VCS, called "B".
- Edit the local path, changing "/var/repo/B" to "/var/repo/A".
- Now it points at a working copy of a repository you can't see.
- Although you won't be able to make it through discovery (the pull will fail with the wrong credentials), you can read some information out of the repository directly through the Diffusion UI, probably?
I'm not sure this was really practical to execute since there are a bunch of sanity checks along most/all of the major pathways, but lock it down since normal users shouldn't be editing it anyway. In the best case, this would make a mess.
Test Plan: {F81391}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D7580
Summary:
Ref T4039. This is mostly to deal with that, to prevent the security issues associated with mutable local paths. The next diff will lock them in the web UI.
I also added a confirmation prompt to `bin/repository delete`, which was a little scary without one.
See one comment inline about the `--as` flag. I don't love this, but when I started adding all the stuff we'd need to let this transaction show up as "Administrator" it quickly got pretty big.
Test Plan: Ran `bin/repository edit ...`, saw an edit with a transaction show up on the web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D7579
Summary:
Fixes T4095. Fixes T3817.
- The batch editor has some funky handle code which misses projects, share that.
- Remove some hacks for T3817 that should be good now.
Test Plan: Looked at batch editor, saw projects. Looked at task list.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, martin.schulz
Maniphest Tasks: T3817, T4095
Differential Revision: https://secure.phabricator.com/D7578
Summary:
Fixes T3741. The flag is respected in terms of actually creating the account, but the UI is a bit unclear.
This can never occur naturally, but installs can register an event which locks it.
Test Plan:
Artificially locked it, verified I got more reasonable UI;
{F81282}
Reviewers: btrahan, datr
Reviewed By: datr
CC: aran
Maniphest Tasks: T3741
Differential Revision: https://secure.phabricator.com/D7577
Summary:
- Add an option for the queue.
- By default, enable it.
- Dump new users into the queue.
- Send admins an email to approve them.
Test Plan:
- Registered new accounts with queue on and off.
- As an admin, approved accounts and disabled the queue from email.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7576
Summary:
- If you're an administrator and there are users waiting for approval, show a count on the home page.
- Sort out the `isUserActivated()` access check.
- Hide all the menu widgets except "Logout" for disabled and unapproved users.
- Add a "Log In" item.
- Add a bunch of unit tests.
Test Plan: Ran unit tests, clicked around as unapproved/approved/logged-in/logged-out users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D7574
Summary:
Nothing fancy here, just:
- UI to show users needing approval.
- "Approve" and "Disable" actions.
- Send "Approved" email on approve.
- "Approve" edit + log operations.
- "Wait for Approval" state for users who need approval.
There's still no natural way for users to end up not-approved -- you have to write directly to the database.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7573
Summary:
Mailbox sometimes (?) changes the case of the email address (?). Be more liberal in what we accept.
Also fix a minor output bug.
Test Plan: Sent mail to `e1+...` instead of `E1+...`, verified it arrived.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7575
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:
- Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
- Migrate all the existing users.
- When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
- Just make the checks look at the `isEmailVerified` field.
- Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
- Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
- When the queue is enabled, registering users are created with `isApproved = false`.
- Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
- They go to the web UI and approve the user.
- Manually-created accounts are auto-approved.
- The email will have instructions for disabling the queue.
I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.
Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.
Test Plan:
- Ran migration, verified `isEmailVerified` populated correctly.
- Created a new user, checked DB for verified (not verified).
- Verified, checked DB (now verified).
- Used Conduit, People, Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7572
Summary:
Ref T3472. Currently, if an install only allows "@mycompany.com" emails and you try to register with an "@personal.com" account, we let you pick an "@mycompany.com" address instead. This is secure: you still have to verify the email. However, it defies user expectation -- it's somewhat confusing that we let you register. Instead, provide a hard roadblock.
(These accounts can still be linked, just not used for registration.)
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3472
Differential Revision: https://secure.phabricator.com/D7571
Summary: See private chatter. Make it explicitly clear when adding a provider that anyone who can browse to Phabricator can register.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7570
Summary: We don't actually support this yet, so hide the configuration.
Test Plan: Edited branches for an hg repo.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7563
Summary:
Ref T2230. As far as I can tell, getting SVN working over HTTP is incredibly complicated. It's all DAV-based and doesn't appear to have any kind of binary we can just execute and pass requests through to. Don't support it for now.
- Disable it in the UI.
- Make sure all the error messages are reasonable.
Test Plan: Tried to HTTP an SVN repo. Tried to clone a Git repo with SVN, got a good error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7562
Summary:
Fixes T3034. This is obsoleted by modern policies.
This was written by a Facebook intern and is rarely used -- the Hive install might be the only use in the wild. It has never really worked correctly.
Test Plan: `grep`; browsed Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3034
Differential Revision: https://secure.phabricator.com/D7568
Summary: Fixes T3535. Also, flip flop on that spacing thing and make the spaces purdy
Test Plan: got an arcanist projected phid in the json dict
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3535
Differential Revision: https://secure.phabricator.com/D7565
Summary: adds FIELD_PROJECTS and deploys it to Maniphest Task Herald Adapter. Went with "projects" because it feels like that could go well in other Adapters that want to conditionalize based on project.
Test Plan: made a new herald rule to be cc'd if project foo was on a task. it worked!
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7564
Summary:
Ref T2230. Very rarely, even though we've flushed the connection and sent all the data, we'll close the connection before Git is happy with it and it will flip out with an error like this:
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed
This is hard to reproduce because it depends on the order of read/write operations we can't directly control. I only saw it about 2% of the time, by just running `git pull` over and over again.
Waiting for Git to close its side of the connection seems to fix it.
Test Plan: Ran `git clone` a ton of times without seeing the error again. Ran `git push` a ton of times with new commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7558
Summary:
Ref T2230. The SVN protocol has a sensible protocol format with a good spec here:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/libsvn_ra_svn/protocol
Particularly, compare this statement to the clown show that is the Mercurial wire protocol:
> It is possible to parse an item without knowing its type in advance.
WHAT A REASONABLE STATEMENT TO BE ABLE TO MAKE ABOUT A WIRE PROTOCOL
Although it makes substantially more sense than Mercurial, it's much heavier-weight than the Git or Mercurial protocols, since it isn't distributed.
It's also not possible to figure out if a request is a write request (or even which repository it is against) without proxying some of the protocol frames. Finally, several protocol commands embed repository URLs, and we need to reach into the protocol and translate them.
Test Plan: Ran various SVN commands over SSH (`svn log`, `svn up`, `svn commit`, etc).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7556
Summary:
Ref T2230. This is substantially more complicated than Git, but mostly because Mercurial's protocol is a like 50 ad-hoc extensions cobbled together. Because we must decode protocol frames in order to determine if a request is read or write, 90% of this is implementing a stream parser for the protocol.
Mercurial's own parser is simpler, but relies on blocking reads. Since we don't even have methods for blocking reads right now and keeping the whole thing non-blocking is conceptually better, I made the parser nonblocking. It ends up being a lot of stuff. I made an effort to cover it reasonably well with unit tests, and to make sure we fail closed (i.e., reject requests) if there are any parts of the protocol I got wrong.
A lot of the complexity is sharable with the HTTP stuff, so it ends up being not-so-bad, just very hard to verify by inspection as clearly correct.
Test Plan:
- Ran `hg clone` over SSH.
- Ran `hg fetch` over SSH.
- Ran `hg push` over SSH, to a read-only repo (error) and a read-write repo (success).
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7553
Summary: Ref T2230. Fixes T4079. As it turns out, this is Git being weird. See comments for some detials about what's going on here.
Test Plan: Created shallow and deep Git clones.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4079, T2230
Differential Revision: https://secure.phabricator.com/D7554
Summary:
Ref T2230. In Git, we can determine if a command is read-only or read/write from the command itself, but this isn't the case in Mercurial or SVN.
For Mercurial and SVN, we need to proxy the protocol that's coming over the wire, look at each request from the client, and then check if it's a read or a write. To support this, provide a more flexible version of `passthruIO`.
The way this will work is:
- The SSH IO channel is wrapped in a `ProtocolChannel` which can parse the the incoming stream into message objects.
- The `willWriteCallback` will look at those messages and determine if they're reads or writes.
- If they're writes, it will check for write permission.
- If we're good to go, the message object is converted back into a byte stream and handed to the underlying command.
Test Plan: Executed `git clone`, `git clone --depth 3`, `git push` (against no-write repo, got error), `git push` (against valid repo).
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, asherkin, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7551
Summary: Missing some `break;`, pretty sure this is causing the issue on `secure.phabricator.com`.
Test Plan: Will push.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7559
Summary: This CAN_EDIT capability doesn't exist. `PhabricatorMacroCapabilityManage::CAPABILITY` (checked on line 15) is used instead.
Test Plan: Disabled, then re-enabled a macro.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Differential Revision: https://secure.phabricator.com/D7550
Summary:
Now that diffs have PHIDs we can create buildables for them.
This also adds `buildable.diff` in the variables list so the diff ID is available, and it also fixes the Cancel button on "Edit Plan" page so it redirects to the right place.
Test Plan: Created a buildable from a diff, ran a build plan against it that had `echo ${buildable.diff}` and got the right ID. Also tested the "Edit Plan" cancel redirect.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7546
Summary:
This uses an event listener to render the status of builds on their buildables. The revision and commit view now renders out the status of each of the builds.
Currently the revision controller has the results for the latest diff rendered out. We might want to show the status of previous diffs in the future, but for now I think the latest diff should do fine.
There's also a number of bug fixes in this diff, including a particularly nasty one where builds would have a build plan PHID generated for them, which resulted in handle lookups always returning invalid objects.
Test Plan: Ran builds against diffs and commits, saw them appear on the revision and commit view controllers.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7544
Summary:
Ref T1049. This is very minimal, but does what it says.
I merged the variable replacement code so Remote + HTTP can share more stuff.
Test Plan:
Ran "HTTP" and "Remote" build plans.
{F79886}
{F79887}
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: zeeg, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7541
Summary: This prevents a crash in applying build plans when more than one buildable exists for the same object. It also adds a check into the "New Manual Build" page to ensure that users can't create a buildable for an object that already has one.
Test Plan: Tried to create a buildable for an object that already has one and a nice friendly error appeared. Applied a build plan to a buildable whose object has two buildables and didn't get a crash any more.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7543
Summary: This adds a `build.id` variable, cleans up the naming convention of other variables and also fixes an issue in the remote command to read the buffers after the command finishes.
Test Plan: Ran a build with `/bin/echo ${build.id}` and saw the build ID come through.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7540
Summary: This puts back the stronger variable replacement that was missed the last update to D7519.
Test Plan: Re-ran a remote build that had variables in the command and everything worked as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7539
Summary: This fixes an issue where content would be discarded when the content to append is larger than the chunk size limit.
Test Plan: Tested running a remote command that does `I=0; while true; do echo "$I"; I=$[$I+1]; done` and all of the outputted numbers matched the line numbers in the logs.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7537
Summary: This adds a build step implementation for running a command on a remote machine over SSH. It supports merging in various variables about the build (such as the commit hash / revision ID, repository call sign, version control type and clone URI).
Test Plan: Configured a build plan to run `/bin/true` on localhost and the build passed. Configured a build plan to run `/bin/false` on localhost and the build failed.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7519
Summary:
Depends on D7519.
This implements support for build logs in Harbormaster. This includes support for appending to a log from the "Run Remote Command" build step.
It also adds the ability to cancel builds.
Currently the build view page doesn't update the logs live; I'm sure this can be achieved with Javelin, but I don't have enough experience with Javelin to actually make it poll from updates to content in the background.
{F79151}
{F79153}
{F79150}
{F79152}
Test Plan:
Tested this by setting up SSH on a Windows machine and using a Remote Command configured with:
```
C:\Windows\system32\cmd.exe /C cd C:\Build && mkdir Build_${timestamp} && cd Build_${timestamp} && git clone --recursive https://github.com/hach-que/Tychaia.git && cd Tychaia && Protobuild.exe && C:\Windows\Microsoft.NET\Framework\v4.0.30319\MSBuild.exe Tychaia.Windows.sln
```
and observed the output of the build stream from the Windows machine into Phabricator.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7521
Summary: these transactions should //never// merge since they are always created for a 1:1 replacement. (ie any merging would be implicitly erroneous). Fixes T4081
Test Plan: made a mock with three images and replaced all three successfully. replaced image A with image B, did not save, replaced image B with image C, then saved and verified transaction correctly showed image A replaced with image C.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4081
Differential Revision: https://secure.phabricator.com/D7536
Summary:
Depends on D7500.
This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).
Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, hwinkel
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7501
Summary: another little piece here that basically just adds some permissions to source editing. serving it up before I do anything too complicated to make sure it seems kosher. in terms of what comes next this form needs to be dynamic based on source type so there'll be some fun there. That said, I plan to implement a more simple "phabricator form" only version to start here and flesh out a few other things like queues with that.
Test Plan: set permission to no one for source edit and got a nice error page.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7535
Summary: I've kept this as close as possible to the Git version for ease of review and later refactoring of them both together. At minimum, the functions to get the working dir should probably be cleaned up one day.
Test Plan: Landed a revision.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7534
Summary:
See <https://github.com/facebook/phabricator/issues/433>. We were missing a "^" here.
This should be moved over to transactions soon and then we can get rid of the duplication. :/
Test Plan: Tried to create a repository with callsign "9X", got a helpful error about "ALL UPPERCASE LETTERS".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7531
Summary:
After upgrading to PHP 5.5, the conduit list was not fully visible because
INF was being treated as "0" for some reason. Fixed by making it a PHP_MAX_INT
Test Plan: Checked on PHP 5.5
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7530
Summary:
Fixes T4067. The way `DiffusionCommitQuery` works prevents it from loading SVN identifiers in some cases without additional constraints, since "12345" might be an SVN revision 12345, or it might be the first 5 characters of a Git commit hash.
Introduce `withRepository()` as a shorthand for `withDefaultRepository()` + `withRepositoryIDs()`. This tells the query to:
- Only look in the given repository; and
- use the more liberal identifier resolution rules while doing so.
The practical impact this has is that blame tooltips in SVN work again. The other queries which are fixed here were never run in SVN (which doesn't have first-class branches or tags); I've cleaned them up only for completeness.
Test Plan:
- Viewed blame in SVN, saw information again instead of empty tooltip.
- Viewed brnaches/tags in Mercurial and Git.
{F79226}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4067
Differential Revision: https://secure.phabricator.com/D7523
Summary:
we were just checking if projects/ was in the URI before barfing. Use some more fun utility functions such that we only complain if there is no project.
Fixes T4071.
Test Plan: made a subpage under a project - success! tried to make a project wiki page where there was no project - successful failure! tried to make a project wiki sub page where there was no project - successful failure!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4071
Differential Revision: https://secure.phabricator.com/D7527
Summary:
See discussion in <https://github.com/facebook/phabricator/issues/430>.
(If we end up with more than like 5 of these we should probably make this a warning or something instead, the only goal is to prevent user error.)
Test Plan:
{F79196}
{F79197}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, enko
Differential Revision: https://secure.phabricator.com/D7522
Summary: The "Reviewers" condition in Differential Revision rules has the wrong typeahead and can't select projects, but should be able to.
Test Plan: {F79273}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7526
Summary: Ref T2230. This is easily the worst thing I've had to write in a while. I'll leave some notes inline.
Test Plan: Ran `hg clone http://...` on a hosted repo. Ran `hg push` on the same. Changed sync'd both ways.
Reviewers: asherkin, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7520
Summary: This is starting to get a bit sizable and it turns out Mercurial is sort of a beast, so split the VCS serve stuff into a separate controller.
Test Plan: Pushed and pulled an authenticated Git repository.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, hach-que
Differential Revision: https://secure.phabricator.com/D7494
Summary: Ref T4068. Partly, this moves discovery to the more unit-testable PhabricatorRepositoryDiscoveryEngine. It also fixes some issues, see inlines.
Test Plan: In a Mercurial repository, ran `bin/repository discover --repair`, verified commits came out topographically sorted. Ran without `--repair` and in various other contexts, like with no commits to discover and some-but-not-all commits to discover.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4068
Differential Revision: https://secure.phabricator.com/D7518
Summary:
I updated the wiki too - https://secure.phabricator.com/w/projects/pebkac/ - with what I am thinking right now. Rough plan here is
- next diff:
- implement editors and transactions
- implement "web type" for contact source
- /pebkac/item/new/ will be the entry point for this
- implement "actions" on a contact
- probably some "polish" on the scaffolding laid out here; like "create" permissions maybs
- diffs after that:
- implement "twitter" type for source
- implement email reply handler stuff for item and source
Probs a great time to blast huge holes in all this stuff. :D
Test Plan: these pages load and arc lint doesn't complain
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7465
Summary:
Ref T1049. Ref T2222. `DifferentialDiff` does not currently have a PHID, but we need it for Harbormaster and ApplicationTransactions. See some discussion in D7501.
(I split the SQL into two sections so we can't fail in the middle. At some point, I'd like to do a pass on the migration stuff and get this happening automatically, and also simplify the PatchList.)
Test Plan:
- Ran `bin/storage upgrade`.
- Checked for valid PHIDs in the database.
- Used `phid.query` to look up a diff by PHID.
- Created a new diff and verified it got a PHID.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2222, T1049
Differential Revision: https://secure.phabricator.com/D7513
Summary: Ref T4068. Adds a command to list all commits in an "importing" status. This will allow users to use `reparse.php` to diagnose and repair issues.
Test Plan:
- Ran `bin/repository importing P`, etc.
- Used `reparse.php` to reparse some commit stages and saw status update correctly.
- Ran on a repo with no importing commits.
- Ran with `... --simple | xargs`, which saves us having to put an `awk` or something in there for users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4068
Differential Revision: https://secure.phabricator.com/D7515
Summary:
Ref T4068. In some cases like that one, I anticipate a repository not fully importing when a handful of random commits are broken. In the long run we should just deal with that properly, but in the meantime provide an administrative escape hatch so you can mark the repository as imported and get it running normally.
The major reason to do this is that Herald, Feed, Harbormaster, etc., won't activate until a repository is "imported".
Test Plan:
- Tried to mark an imported repository as imported, got an "already imported" message.
- Same for not-imported.
- Marked a repository not-imported.
- Marked a repository imported.
- Marked a repository not-imported, then waited for the daemons to mark it imported again automatically.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, kbrownlees
Maniphest Tasks: T4068
Differential Revision: https://secure.phabricator.com/D7514
Summary: Ref T1049. Nothing fancy, but shows red for fail/error and green for pass. See discussion in D7502.
Test Plan: {F78839}
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7512
Summary:
Depends on D7501.
This just renders the buildable's actual object name onto the list, so you can see at a glance what the buildable represents. I'd like to also pull across a list of builds of this buildable and change the bar color, but I'm not quite sure how to do that in the search architecture without N+1 querying.
Test Plan:
Looked at the buildable list and it looked like this:
{F78555}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7502
Summary: Cleans up some CSS while adding lots of other... Mainly, this allow min-width "tables" that trigger a scroll-bar, but go full width if larger than min.
Test Plan: Tested Workboard Examples and some Project pages, Chrome, Tablet and Mobile Layouts
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7509
Summary: This is a little funky but fixes an issue with Git repos that are
non-bare needing "origin/" to resolve branches other than "master". Eventually
this should get cleaned up.
Test Plan: Reporting user verified this fixed their issue.
Auditors: btrahan
Summary: Ref T4064. The response code here isn't normally relevant, but we can hit these via `git clone http://../`, etc., and it's clearly more correct to use HTTP 500.
Test Plan: Added a fake `throw new Exception()` and verified I got an HTTP 500 response.
Reviewers: jamesr, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4064
Differential Revision: https://secure.phabricator.com/D7507
Summary: This implements an interface for adding new build steps, editing existing build steps and deleting build steps from build plans. It uses the settings definitions on the build implementation to work out what fields should be displayed on the edit page.
Test Plan:
See screenshots:
{F78529}
{F78532}
{F78528}
{F78531}
{F78527}
{F78530}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7500
Summary:
Depends on D7498.
This implements support for a "build step implementation". Build steps have an associated class name (which makes the class in PHP) and a details field, which is serialized JSON (same as PhabricatorRepository).
This also implements a SleepBuildStepImplementation which just pauses the build for a specified period of seconds.
Test Plan:
Inserted a build step with `insert into harbormaster_buildstep (phid, buildPlanPHID, className, details, dateCreated, dateModified) values ('', 'PHID-HMCP-zkh5w6czfbfpk2gxwdeo', 'SleepBuildStepImplementation', '{"seconds":5}', NOW(), NOW());` (adjusting the build plan PHID as appropriate).
Started the daemon and applied the build plan to a buildable, and saw the daemon take a 5 second delay after creating `SleepBuildStepImplementation`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7499
Summary:
ref T182.
Simple approach of clone, patch, push. While waiting for drydock, implement a hackish mutex
setup for the workspace, which should work ok as long as there's only one committer who is
carefull about theses things.
Less obvious note: This is taking the both author and commiter's 'primary email' for the commit -
which might rub some people wrong.
Test Plan:
With a hosted repo, created some diffs and landed them.
Also clicked button for some error cases, got the right error message.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: hach-que, Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7486
Summary: This implements a basic Harbormaster daemon that takes pending builds and builds them (currently just sleeps 15 seconds before moving to passed state). It also implements an interface to apply a build plan to a buildable, so that users can kick off builds for a buildable.
Test Plan: Ran `bin/phd debug PhabricatorHarbormasterBuildDaemon` and used the interface to start some builds by applying a build plan. Observed them move from 'pending' to 'building' to 'passed'.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7498
Summary:
Fixes T4060. The logic here is:
- When you take several actions at once, we show a single feed story for all of them.
- We choose the "most interesting" title for the feed story. For example, "close task" is more interesting than "add CC".
Currently, the issue with this is:
- "Add comment" is the //least interesting// title. I think this is correct: all other actions are more interesting than the fact that you added a comment.
- We try to conserve the number of objects we need to load by rendering only the most interesting transaction.
To fix this:
- Stop being so conservative; load all of the transactions and all of their PHIDs.
- Add bodies from any transactions which render bodies. In all cases (I think?) this is a maximum of one comment adding a body.
The end result is a story like this:
epriestley closed T123: the building is on fire.
"Okay guys I put the fire out"
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran, asherkin
Maniphest Tasks: T4060
Differential Revision: https://secure.phabricator.com/D7504
Summary:
Expands on D7488, which looks way better than the config checks. I'm leaving the config checks for now, but maybe we should just get rid of them? This advice is delivered in a far more timely way.
- Check for normal VCS binaries too.
- Link to `environment.append-paths`.
- Get rid of untranslated names (I think they're probably not too useful?)
Test Plan: See screenshots.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Differential Revision: https://secure.phabricator.com/D7495
Summary:
Currently if 'git-http-backend' is not on the PATH, there is no visible message to the user other than "info/refs: is this a valid git repository?" when trying to clone. This adds a setup check so that if there are any Git repositories in use, it will check for the existance of the "git-http-backend" binary in the PATH.
I believe this is shipped by default alongside the git package on most distros, but in some (such as OpenSUSE), this binary isn't on the PATH by default.
Test Plan: Removed `/usr/lib/git` from my `environment.append-paths` and saw the message appear. Added it back and the message went away.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4050
Differential Revision: https://secure.phabricator.com/D7488
Summary:
Ref T1493. Diffusion has some garbagey behavior for things we can't resolve. Common cases are:
- Looking at a branch that doesn't exist.
- Looking at a repository with no branches.
- Looking at a commit that doesn't exist.
- Looking at an empty repository.
In these cases, we generally fatal unhelpfully. I want to untangle this mess.
This doesn't help much, but does clean things up a bit. We currently have two separate query paths, "stablecommitname" and "expandshortcommit". These are pretty much doing the same thing -- taking some ref like "master" or "default" or a tag name or part of a commit name, and turning it into a full commit name. Merge them into a single "resolverefs" method.
This simplifies the code a fair bit, and gives us better error messages. They still aren't great, but they're like this now:
Ref "7498aec194ecf2d333e0e2baddd9d5cdf922d7f1" is ambiguous or does not exist.
...instead of just:
ERR-INVALID-COMMIT
Test Plan: Looked at Git, Mercurial and Subversion repositories that were empty and non-empty. Looked at branches/heads. Tried to look at invalid commits. Looked at tags. All of this still works, and some behaviors are a bit better than they used to be.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D7484
Summary: Fixes the junk I broke in D7484. Before that, tag content was a side effect of resolving the ref name. Now, fetch it explicitly in `diffusion.tagsquery`.
Test Plan: Looked at a tag, saw the annotation/message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7485
Summary: This puts Conduit calls into the "Services" tab. They aren't always real service calls, but I think they're big enough to belong there and be useful.
Test Plan: Viewed "Services" tab, saw conduit calls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7482
Summary: Ref T1493. Consolidate these a bit; they might need some more magic once we do `--noupdate` checkouts. Mostly just trying to clean up and centralize this code a bit.
Test Plan: Viewed and `bin/repository discover`'d Mercurial repos with and without any branches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D7480
Summary: The idea is to have all `phtize` definitions in applications to allow their separation.
Test Plan: Clicked View Options after mangling the translation.
Reviewers: epriestley
Reviewed By: epriestley
CC: btrahan, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7345
Summary: Adds summary (description) and test plan icons to make these area's more unique and differentiated over general sections.
Test Plan: Test a diff, a commit, a task
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7493
Summary: This disables CSRF checking around the `$repository->writeStatusMessage` so that pushing changes over HTTP to Git repositories doesn't fail miserably.
Test Plan: Applied this fix and I could `git push` to hosted repositories again.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4052
Differential Revision: https://secure.phabricator.com/D7490
Summary: This fixes an issue where Git authentication would always fail on an install with `policy.allow-public` set to false. This is because when public access is allowed, anonymous users can query the user list. However, when public access is not allowed, you have to be authenticated before you can read any of the user objects.
Test Plan:
Prior to this fix, I get:
```
james@james-laptop:~/git/8> git clone http://phabricator.local/diffusion/TEST/
Cloning into 'TEST'...
fatal: unable to access 'http://phabricator.local/diffusion/TEST/': The requested URL returned error: 403
```
when `policy.allow-public` is false. After this fix I get:
```
james@james-laptop:~/git/8> git clone http://phabricator.local/diffusion/TEST/
Cloning into 'TEST'...
remote: Counting objects: 102, done.
remote: Compressing objects: 100% (71/71), done.
remote: Total 102 (delta 6), reused 0 (delta 0)
Receiving objects: 100% (102/102), 9.89 KiB | 0 bytes/s, done.
Resolving deltas: 100% (6/6), done.
Checking connectivity... done
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4049
Differential Revision: https://secure.phabricator.com/D7489
Summary: This implements Conduit calls for querying Phame blogs and Phame posts.
Test Plan: Made some calls and they seem to generally work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3695
Differential Revision: https://secure.phabricator.com/D7478
Summary: This adds support for a Conduit method to process Remarkup content in bulk. It also updates the `getEngineContexts` methods to support any missing contexts.
Test Plan: Ran the command and processed a few sets of text.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4046
Differential Revision: https://secure.phabricator.com/D7479
Summary: This adds pht's and such english to Phriction email body.
Test Plan: Edited a Document, Moved a Document. Got new emails. Such Wow.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7477
Summary:
Ref T2230. This will need some more refinement, but basically it adds a "Create" vs "Import" step before we go through the paged workflow.
- If you choose "Create", we skip the remote URI / auth stuff, and then set the "hosted" flag.
- If you choose "Import", we do what we do now.
Test Plan: Created and imported repos.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7475
Summary:
Hosted repositories only sometimes survive the pull/discover phases right now, due to issues like:
- Pull tries to `git clone`, but should `git init`.
- Mercurial doesn't handle empty repositories with on branches.
- SVN tries to connect to an invalid remote.
- None of them set the INIT repo flag correctly, so status doesn't get updated properly in the UI.
Fix all this stuff.
Test Plan:
- For each of Git, SVN and Mercurial:
- Created a new repository from the web UI in a deactivated state.
- Made it hosted.
- Manually ran pull/discover.
- Verified we end up with initialized, empty repositories in consistent states.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7474
Summary:
- Warn about "Read/Write" instead of disabling it, to prevent edits which mutate it after changing a hosted repository to an unhosted one.
- Warn about authenticated connections with HTTPS auth disabled, and link to the relevant setting.
- When "Autoclose" is disabled, show that "Autoclose Branches" won't have an effect.
- For hosted repositories, show the HTTP and SSH clone URIs.
- Make them easy to copy/paste.
- Link to credential management.
- Show if they're read-only.
- This could be a bit nicer-looking than it is.
Test Plan: Looked at repositories in a bunch of states and made various edits to them.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7471
Summary:
See <https://github.com/facebook/phabricator/issues/425>. There are some ways that the change parsers may not reach `finishParse()`, but we now need them to in order to mark the commit imported, advance the progress bar, and eventually kick the repository out of IMPORTING status.
Take all the copy/pasted code in the parsers and move it into the parent. Specifically, this is:
- Printing a status message about starting a parse;
- checking for bad commits;
- queueing the next parse stage; and
- marking the import step complete.
Test Plan: Used `reparse.php --change` to reparse Git, SVN and Mercurial repos.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7470
Summary: Depends on D7642. This updates the authentication logic so that HTTP writes can be made to Git repositories hosted by Phabricator.
Test Plan: Set the policy to allow me to push and I was able to. Changed the policy to disallow push and I was no longer able to push.
Reviewers: #blessed_reviewers, hach-que
Reviewed By: hach-que
CC: Korvin, epriestley, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7468
Summary: This allows users to set their HTTP access passwords via Diffusion interface.
Test Plan: Clicked the "Set HTTP Access Password" link, set a password and saw it appear in the DB.
Reviewers: #blessed_reviewers, hach-que, btrahan
Reviewed By: hach-que
CC: Korvin, epriestley, aran, jamesr
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7462
Summary:
Ref T2350. Fixes T2231.
- Adds log flags around discovery.
- Adds message flags for "needs update". This is basically an out-of-band hint to the daemons that a repository should be pulled sooner than normal. We set the flag when users push a revision, and expose a Conduit method that `arc land` will be able to use.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2350, T2231
Differential Revision: https://secure.phabricator.com/D7467
Summary:
I pulled these into the property list recently, which made them more consistent, but that dropped "preserve linebreaks". Since these usually come from the CLI, render with linebreaks preserved.
@csilvers, you'll need to `bin/cache purge --purge-remarkup` after this if you want to fix existing revisions.
Test Plan: Made a revision with some poetry, saw poetry preserved.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: csilvers, aran
Differential Revision: https://secure.phabricator.com/D7464
Summary: At least under GitHub, the token value is stored as "null", and not missing. And `null > anything` is false, so Phabricator thinks the token is expired or not there.
Test Plan: http://ph.vm/settings/panel/external/ before shows "No OAuth Access Token," and after it says "Active OAuth Token".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7466
Summary:
`RepositoryStatusMessage` is basically a key/value table associated with a repository that I'm using to let the daemons store the most recent event of a given type, so we can easily show it on the status dashboard. I think this will be a lot easier for users to figure out than digging through logfiles.
I'm also going to write the "this needs a pull" status here eventually, for reducing the time lapse between pushes and discovery.
- Add storage for these messages.
- Have the pull engine populate the INIT phase. I'll do the FETCH phase next.
- Update the status readout to show all the various states.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7461
Summary: This moved into Diffusion in D7458 and is now presented in a much cleaner, more targeted way.
Test Plan: Loaded `/repository/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7459
Summary:
Replace the blanket "daemons not running" warning with a lot more specific detail, to try to make it easier for users to figure out how to set up repositories correctly.
The next change here will add some additional status information from the daemons, so this panel can report results in greater detail.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7458
Summary:
- Use DiffusionCommitQuery
- Get rid of the "Author" column.
- Collapse commit + revision together.
- Better tooltips to cover for the removed information.
- Colorize only the "line" column.
- Generally, reduce the amount of visual noise and non-code-stuff going on in this interface.
- I'd like to make the "<<" thing look nicer too but that might take some actual design.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7457
Summary: Minor cleanup. Make the "imported" check less strict (we don't need owners or herald to show change status). Export the "imported" flag over Conduit.
Test Plan: Viewed tag table. Viewed partially imported repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7455
Summary: Swap to DiffusionCommitQuery, other minor cleanup.
Test Plan: Viewed page, forced error view and looked at it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7454
Summary: See @scottmac's reply in T3982. It looks like his email client uses the standard quote string, but includes it in the quoted block.
Test Plan: Added a failing unit test, made it pass.
Reviewers: btrahan
Reviewed By: btrahan
CC: scottmac, aran
Differential Revision: https://secure.phabricator.com/D7440
Summary: I'm planning to add more detailed info to Diffusion itself, but catch the big issue here.
Test Plan: Hit config issue locally, then resolved it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7439
Summary: The new "importStatus" property provides a much stronger and more consistent version of this flag. The only callsite was removed by D7452.
Test Plan: Used `grep` to check for callsites and found none.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7453
Summary:
Ref T2716.
- Serve from `DiffusionCommitQuery`, not `PhabricatorAuditCommitQuery` (which should probably die).
- Fix logic for `limit`, which incorrectly failed to display the "Showing %d branches." text.
- Clean up things a touch.
- I didn't end up actually needing `needCommitData()`, but left it in there since I think it will be needed soon.
- Removed a "TODO" because I don't remember what "etc etc" means.
Test Plan: Looked at branches in several repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2716
Differential Revision: https://secure.phabricator.com/D7451
Summary: If you load Diffusion between a repository being pulled and discovered, you can end up with a valid commit reference that hasn't been discovered yet. Don't fatal.
Test Plan: Saw somewhat-helpful error page instead of fatal.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7448
Summary: Ref T2230. This cleans up D7442, by using `git for-each-ref` everywhere we can, in a basically reasonable way.
Test Plan:
In bare and non-bare repositories:
- Ran discovery with `bin/repository discover`;
- listed branches on `/diffusion/X/`;
- listed tags on `/diffusion/X/`;
- listed tags, branches and refs on `/diffusion/rXnnnn`.
Reviewers: btrahan, avivey
Reviewed By: avivey
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7447
Summary: Fixes T4035. I removed these two "remote/" things in rP59922b7, but we need them for non-bare repositories. Without them, the commands work and run fine and the output looks OK, but the results may not reflect the correct information (e.g., the log shows the working copy's master, which may not be in the same state as origin/master). I'm going to generally clean this up, but unbreak it for now.
Test Plan: Viewed bare and non-bare repositories in Diffusion, got accurate history.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T4035
Differential Revision: https://secure.phabricator.com/D7445
Summary: Makes Legalpad a little easier to grok.
Test Plan: View Document
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7460
Summary: The warning panel on large commits in diffusion was being overrun with other styles. Fixes T3952
Test Plan: test on a large commit
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3952
Differential Revision: https://secure.phabricator.com/D7456
Summary: We don't have a section header on `/diffusion/X/` for descriptions right now. Add one to improve consistency.
Test Plan: Looked at a repository.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7449
Summary: Helps make it seem more q/a like and consistent.
Test Plan: Look at question
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7450
Summary:
Ref T2230. Although all the non-bare commands //run// fine in bare repos, not all of them do exactly the same thing.
This could use further cleanup, but at least get it working again for now.
Test Plan: Ran `bin/repository pull`, `bin/repository discover`, viewed Diffusion (looked at branch table), viewed a commit (looked at "Branches"), for bare and non-bare git repos.
Reviewers: avive, btrahan, avivey
Reviewed By: avivey
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7442
Summary: Updates the review status list to align better inside property lists. Alsu uses the default colors a bit more. This removes an overflow hidden on the value side, but that shouldnt cause any issues, given it has plenty of space.
Test Plan: tested differential and audit, highlighted and not.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7441
Summary:
Relocated files aren't treated as newly created files by the worker. This
can lead to the worker trying to look up information about deleted files
in the wrong location.
Test Plan: See T4030
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Maniphest Tasks: T4030
Differential Revision: https://secure.phabricator.com/D7432
Summary: Fixes T4028, makes the table a bit easier to read and navigate changesets.
Test Plan: Tested a big history, back to original and latest.
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran
Maniphest Tasks: T4028
Differential Revision: https://secure.phabricator.com/D7438
Summary:
Fixes T3619. These URIs are valid:
git@domain.com:/path (Git SCP-style implicit SSH)
ssh://git@domain.com/path (Explicit SSH)
This URI, arrived at by adding "ssh://" to the front of an SCP-style URI, is not:
ssh://git@domain.com:/path
Detect URIs in this form and reject them. See T3619.
Test Plan:
{F75486}
Also set some valid URIs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3619
Differential Revision: https://secure.phabricator.com/D7431
Summary:
This doesn't really impact anything very much, but is a little cleaner than cloning repositories with a working copy. It's somewhat important for allowing pushes, because you can't push to a checked-out branch.
Mercurial has a similar option (`--noupdate`) but leave that alone for now.
The origin stuff was mostly for sanity/explicitness purposes -- I believe it's safe to remove in all non-ridiculous cases. Git fails with it in bare repositories (it automatically creates an `origin`, but doesn't create the local refs for it, or something).
Test Plan: Nuked a repo, re-cloned it, pulled and updated it several times. Browsed both bare and non-bare repos in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7430
Summary:
Fixes T3217. Ref T776. Ref T1493. Broadly, this introduces a mechanism which works like this:
- When a repository is created, we set an "importing" flag.
- After discovery completes, we check if a repository has no importing commits. Basically, this is the first time we catch up to HEAD.
- If we're caught up, clear the "importing" flag.
This flag lets us fix some issues:
- T3217. Currently, when you import a new repository and users have rules like "Email me on every commit ever" or "trigger an audit on every commit", we take a bunch of publish actions. Instead, implicitly disable publishing during import.
- An imported but un-pulled repository currently has an incomprehensible error on `/diffusion/X/`. Fix that.
- Show more cues in the UI about importing.
- Made some exceptions more specific.
Test Plan:
This is the new screen for a completely new repo, replacing a giant exception:
{F75443}
- Created a repository, saw it "importing".
- Pulled and discovered it.
- Processed its commits.
- Ran discovery again, saw import flag clear.
- Also this repository was empty, which hit some of the other code.
This is the new "parsed empty repository" UI, which isn't good, but is less broken:
{F75446}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, hach-que
Maniphest Tasks: T3607, T1493, T776, T3217
Differential Revision: https://secure.phabricator.com/D7429
Summary:
Fixes T3416. Fixes T1733.
- Adds a flag to the commit table showing whether or not we have parsed it.
- The flag is set to `0` initially when the commit is discovered.
- The flag is set to `1` when the changes are parsed.
- The UI can now use the flag to distinguish between "empty commit" and "commit which we haven't imported changes for yet".
- Simplify rendering code a little bit.
- Fix an issue with the Message parser for empty commits.
- There's a key on the flag so we can do `SELECT * FROM repository_commit WHERE repositoryID = %d AND importStatus = 0 LIMIT 1` soon, to determine if a repository is fully imported or not. This will let us improve the UI (Ref T776, Ref T3217).
Test Plan:
- Ran `bin/storage upgrade -f`.
- Created an empty commit.
- Without the daemons running, ran `bin/repository pull GTEST` and `bin/repository discover GTEST`.
- Viewed web UI to get the first screenshot ("Still Importing...").
- Ran the message and change steps with `scripts/repository/reparse.php`.
- Viewed web UI to get the second screenshot ("Empty Commit").
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T776, T1733, T3416, T3217
Differential Revision: https://secure.phabricator.com/D7428
Summary:
- Don't try to pull hosted repos.
- Also, fix the `--verbose` + `--trace` interaction for `bin/repository`.
- Also, fix a couple of unit tests which got tweaked earlier.
Test Plan:
$ ./bin/repository pull GTEST --verbose
Pulling 'GTEST'...
Repository "GTEST" is hosted, so Phabricator does not pull updates for it.
Done.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7427
Summary: We've had support for this for a long time, but it was conditional on config. Since it more-or-less actually does something now, just enable it unconditionally.
Test Plan: Settings -> SSH Public Keys
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7426
Summary: Looks like this is pretty straightforward; same as the reads except mark it as needing PUSH.
Test Plan: Ran `git push`, pushed over SSH to a hosted repo.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7425
Summary: Like D7423, but for SSH.
Test Plan: Ran `git clone ssh://...`, got a clone.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7424
Summary: Mostly ripped from D7391. No writes yet.
Test Plan: Ran `git clone` against a local over HTTP, got a clone.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7423
Summary:
- Add web UI for configuring SSH hosting.
- Route git reads (`git-upload-pack` over SSH).
Test Plan:
>>> orbital ~ $ git clone ssh://127.0.0.1/
Cloning into '127.0.0.1'...
Exception: Unrecognized repository path "/". Expected a path like "/diffusion/X/".
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
>>> orbital ~ $ git clone ssh://127.0.0.1/diffusion/X/
Cloning into 'X'...
Exception: No repository "X" exists!
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
>>> orbital ~ $ git clone ssh://127.0.0.1/diffusion/MT/
Cloning into 'MT'...
Exception: This repository is not available over SSH.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
>>> orbital ~ $ git clone ssh://127.0.0.1/diffusion/P/
Cloning into 'P'...
Exception: TODO: Implement serve over SSH.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7421
Summary:
Fixes T2229. This sets the stage for a patch similar to D7417, but for SSH. In particular, SSH 6.2 introduced an `AuthorizedKeysCommand` directive, which lets us do this in a mostly-reasonable way without needing users to patch sshd (if they have a recent enough version, at least).
The way the `AuthorizedKeysCommand` works is that it gets run and produces an `authorized_keys`-style file fragment. This isn't ideal, because we have to dump every key into the result, but should be fine for most installs. The earlier patch against `sshd` passes the public key itself, which allows the script to just look up the key. We might use this eventually, since it can scale much better, so I haven't removed it.
Generally, auth is split into two scripts now which mostly do the same thing:
- `ssh-auth` is the AuthorizedKeysCommand auth, which takes nothing and dumps the whole keyfile.
- `ssh-auth-key` is the slightly cleaner and more scalable (but patch-dependent) version, which takes the public key and dumps only matching options.
I also reworked the argument parsing to be a bit more sane.
Test Plan:
This is somewhat-intentionally a bit obtuse since I don't really want anyone using it yet, but basically:
- Copy `phabricator-ssh-hook.sh` to somewhere like `/usr/libexec/openssh/`, chown it `root` and chmod it `500`.
- This script should probably also do a username check in the future.
- Create a copy of `sshd_config` and fix the paths/etc. Point the KeyScript at your copy of the hook.
- Start a copy of sshd (6.2 or newer) with `-f <your config file>` and maybe `-d -d -d` to foreground and debug.
- Run `ssh -p 2222 localhost` or similar.
Specifically, I did this setup and then ran a bunch of commands like:
- `ssh host` (denied, no command)
- `ssh host ls` (denied, not supported)
- `echo '{}' | ssh host conduit conduit.ping` (works)
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2229, T2230
Differential Revision: https://secure.phabricator.com/D7419
Summary:
Mostly ripped from D7391, with some changes:
- Serve repositories at `/diffusion/X/`, with no special `/git/` or `/serve/` URI component.
- This requires a little bit of magic, but I got the magic working for Git, Mercurial and SVN, and it seems reasonable.
- I think having one URI for everything will make it easier for users to understand.
- One downside is that git will clone into `X` by default, but I think that's not a big deal, and we can work around that in the future easily enough.
- Accept HTTP requests for Git, SVN and Mercurial repositories.
- Auth logic is a little different in order to be more consistent with how other things work.
- Instead of AphrontBasicAuthResponse, added "VCSResponse". Mercurial can print strings we send it on the CLI if we're careful, so support that. I did a fair amount of digging and didn't have any luck with git or svn.
- Commands we don't know about are assumed to require "Push" capability by default.
No actual VCS data going over the wire yet.
Test Plan:
Ran a bunch of stuff like this:
$ hg clone http://local.aphront.com:8080/diffusion/P/
abort: HTTP Error 403: This repository is not available over HTTP.
...and got pretty reasonable-seeming errors in all cases. All this can do is produce errors for now.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7417
Summary:
Basically straight from D7391. The differences are basically:
- Policy stuff is all application-scope instead of global-scope.
- Made a few strings a little nicer.
- Deleted a bit of dead code.
- Added a big "THIS DOESN'T WORK YET" warning.
Test Plan: See screenshots.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7416
Summary: No editing or view yet, just adds the schema and a policy default. Part of D7391.
Test Plan: `bin/storage upgrade`
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7415
Summary: Ref T2231. Get rid of the old create controller and make the button go to the new stuff instead. This will eventually get cleaned up more, but I don't have a clear plan for Arcanist Projects yet.
Test Plan: Clicked button, hit new workflow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7414
Summary:
Ref T2231. This:
- Activates the new multi-step workflow, and exposes it in the UI.
- Adds "can create", "default view" and "default edit" capabilities.
- Provides a default value for `repository.default-local-path` and forces repositories into it by default. It's still editable, but Phabricator gets it correct (for some definition of correct) by default now.
Test Plan: Created some new repositories with the new workflow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1286, T2231
Differential Revision: https://secure.phabricator.com/D7413
Summary:
Ref T2231. I didn't port these options over, so they're still supported but have no edit UI:
- Pull Frequency (confusing/not useful, I think?)
- Default Owners Path (probably used only by Facebook and only in the E repository)
- Show user in public repository URL (probably mostly obsolete with hosting?)
We can add those back if users notice, but they seem like the three least useful options so I'm going to see if we can get away with removing them.
Test Plan: Clicked "Edit" from Repositories, got kicked into the nice new Diffusion edit UI instead of the old one.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7410
Summary: Ref T2231. This just moves the "Delete" dialog from Repositories to Diffusion. This dialog just shows instructions and isn't interesting.
Test Plan: {F75093}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7412
Summary: Ref T2231. Use status info element instead of tags.
Test Plan: {F75092}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7411
Summary: Fixes T1286. Ref T2231. See previous diffs; same as the others but does "Local Path".
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1286, T2231
Differential Revision: https://secure.phabricator.com/D7409
Summary: Ref T2231. Crumbs in the Diffusion edit workflow are a bit wonky, with stuff like "rP (master)" which isn't very useful and no link back to the main "Edit" page. Make them consistent across all the screens.
Test Plan: Loaded a bunch of these screens and saw sane crumbs on all of them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7407
Summary:
@chad is hitting an issue described in P961, which I think is this bug in PHP: https://bugs.php.net/bug.php?id=43200
Work around it by defining a "PHIDInterface" and having both "Flaggable" and "Policy" extend it, so that there is only one `getPHID()` declaration.
Test Plan: shrug~
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7408
Summary: Ref T2231. The policy rules are a little murky right now: the "Edit Repository" link requires CAN_EDIT, but the actualy page doesn't. Instead, require CAN_EDIT for the edit page.
Test Plan: As a user without CAN_EDIT, viewed a repository and clicked the edit link.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7406
Summary:
Ref T2231. Allows you to edit the remote URI and credentials.
This is a little bit funky because I'm reusing some of the pages on the new (not-yet-hooked-up) create form. Specifically, it had pages like this:
- Repo Type
- Name/Callsign/Remote
- Auth
- Done
I split "Name/Callsign/Remote" into "Name/Callsign" and "Remote", then when editing the remote I just take you through "Remote" and "Auth" and then back. This lets us reuse the giant pile of protocol/URI sanity checking logic and ends up being pretty clean, although it's a little weird that the "Create" controller does both full-create and edit-remote.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7405
Summary: Ref T2231. Brings "Notify/Publish" and "Autoclose" to the new UI.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D7402
Summary: also add a few more words
Test Plan: looks good!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7404
Summary: See title. Fixes T1809.
Test Plan:
verified each type that has flaggable interface still can be flagged
verified that new custom query filter works
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D7392
Summary: Some scripts might find it easier to work with PHIDs instead of user names.
Test Plan:
Use ?assign=<username> and ?assign=<PHID-USER> with the create task URI.
See assignee input being filled correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin
Differential Revision: https://secure.phabricator.com/D7401
Summary: First cut of an 'info panel' for phabricator. Basic concept is for display a list of items with a bit more info and depth and an object item list. Projects could be a good first example.
Test Plan: UIExamples
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7398
Summary:
Projects and priority inputs can be prefilled similar to how title
and description fields work.
Prefilling of projects already worked but used PHIDs instead of
more user friendly name so I changed that too.
Test Plan:
Visit [[/maniphest/task/create/?projects=Maniphest;Easy&priority=100&assign=vrana&title=Hip-hip&description=hooray!|example]]
and see prefilled form fields.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7394
Summary:
Gets rid of some old Differential-specific nonsense and replaces it with general runtime-pluggable Remarkup rules.
Facebook: This removes two options which may be in use. Have any classes being added via config here just subclass the new abstract bases instead. This should take 5 seconds to fix. You can adjust order by overriding `getPriority()` on the rules, if necessary.
Test Plan: See comments.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, andrewjcg, aran
Differential Revision: https://secure.phabricator.com/D7393
Summary: More Diffusion/Differential touch ups, ToC, etc.
Test Plan: Look at colors, see that they match or look better.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, epriestley, aran
Maniphest Tasks: T3952
Differential Revision: https://secure.phabricator.com/D7386
Summary: Changes to checkmark and crossed circle to match active projects
Test Plan: installed and uninstalled an application. poor conpherence.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7390
Summary:
This code is a little funky right now, and can return `array("error message")` and then try to call `getHunks()` on it. Additionally, each field loads the commit's changes separately.
Instead, load the commit's changes once and cache them, and handle exceptions appropriately.
Test Plan:
- Created a rule like "changed, added, removed content all match /.*/" to force all fields to generate.
- Ran it successfully.
- Faked an error and ran it, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: bigo, aran
Differential Revision: https://secure.phabricator.com/D7384
Summary:
Ref T1049. I don't really want to sink too much time into this right now, but a seemingly reasonable architecture came to me in a dream. Here's a high-level overview of how things fit together:
- **"Build"**: In Harbormaster, "build" means any process we want to run against a working copy. It might actually be building an executable, but it might also be running lint, running unit tests, generating documentation, generating symbols, running a deploy, setting up a sandcastle, etc.
- `HarbormasterBuildable`: A "buildable" is some piece of code which build operations can run on. Generally, this is either a Differential diff or a Diffusion commit. The Buildable class just wraps those objects and provides a layer of abstraction. Currently, you can manually create a buildable from a commit. In the future, this will be done automatically.
- `HarbormasterBuildStep`: A "build step" is an individual build operation, like "run lint", "run unit", "build docs", etc. The step defines how to perform the operation (for example, "run unit tests by executing 'arc unit'"). In this diff, this barely exists.
- `HarbormasterBuildPlan`: This glues together build steps into groups or sequences. For example, you might want to "run unit", and then "deploy" if the tests pass. You can create a build plan which says "run step "unit tests", then run step "deploy" on success" or whatever. In the future, these will also contain triggers/conditions ("Automatically run this build plan against every commit") and probably be able to define failure actions ("If this plan fails, send someone an email"). Because build plans will run commands, only administrators can manage them.
- `HarbormasterBuild`: This is the concrete result of running a `BuildPlan` against a `Buildable`. It tracks the build status and collects results, so you can see if the build is running/successful/failed. A `Buildable` may have several `Build`s, because you can execute more than one `BuildPlan` against it. For example, you might have a "documentation" build plan which you run continuously against HEAD, but a "unit" build plan which you want to run against every commit.
- `HarbormasterBuildTarget`: This is the concrete result of running a `BuildStep` against a `Buildable`. These are children of `Build`. A step might be able to produce multiple targets, but generally this is something like "Unit Tests" or "Lint" and has an overall status, so you can see at a glance that unit tests were fine but lint had some issues.
- `HarbormasterBuildItem`: An optional subitem for a target. For lint, this might be an individual file. For unit tests, an individual test. For normal builds, an executable. For deploys, a server. For documentation generation, there might just not be subitems.
- `HarbormasterBuildLog`: Provides extra information, like command/execution transcripts. This is where stdout/stderr will get dumped, and general details and other messages.
- `HarbormasterBuildArtifact`: Stores side effects or results from build steps. For example, something which builds a binary might put the binary in "Files" and then put its PHID here. Unit tests might put coverage information here. Generally, any build step which produces some high-level output object can use this table to record its existence.
This diff implements almost nothing and does nothing useful, but puts most of these object relationships in place. The two major things you can't easily do with these objects are:
1) Run arbitrary cron jobs. Jenkins does this, but it feels tacked on and I don't know of anyone using it for that. We could create fake Buildables to get a similar effect, but if we need to do this I'd rather do it elsewhere in general. Build and cron/service/monitoring feel like pretty different problems to me.
2) Run parameterized/matrix steps (maybe?). Bamboo has this plan/stage/task/job breakdown where a build step can generate a zillion actual jobs, like "build client on x86", "build server on x86", "build client on ARM", "build server on ARM", etc. We can sort of do this by having a Step map to multiple Targets, but I haven't really thought about it too much and it may end up being not-great. I'd guess we have like an 80% chance of getting a clean implementation if/when we get there. I suspect no one actually needs this, or when they do they'll just implement a custom Step and it can be parameterized at that level. I'm not too worried about this overall.
The major difference between this and Jenkins/Bamboo/TravisCI is that all three of those are **plan-centric**: the primary object in the system is a build plan, and the dashboard shows you all your build plans and the current status. I don't think this is the right model. One disadvantage is that you basically end up with top-level messaging that says "Trunk is broken", not "Trunk was broken by commit af32f392f". Harbormaster is **buildable-centric**: the primary object in the system is stuff you can run build operations against (commits/branches/revisions), and actual build plans are secondary. The main view will be "recent commits on this branch, and whether they're good or not" -- which I think is what's most important in a larger/more complex product -- not the pass/fail status of all jobs. This also makes it easier and more natural to integrate with Differential and Diffusion, which both care about the overall status of the commit/revision, not the current status of jobs.
Test Plan: Poked around, but this doesn't really do anything yet.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, chad, aran, seporaitis
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7368
Summary: Ref T4010. Adds a history page and restores the transaction title strings, which previously sort-of existed in the defunct feed story class.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7371
Summary:
Ref T4010. Projects have a weird proto-version of ApplicationTransactions which is very similar but not quite the same.
Move the storage to a modern format, but keep all the other code for now.
Test Plan: Migrated project transactions; edited projects.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7370
Summary:
Conduit doesn't currently have an analog to "shouldAllowPublic", so the recent policy checks added here caught legitimate Conduit calls when viewing Diffusion as a logged-out user.
Add `shouldAllowPublic()` and set it for all the Diffusion queries.
(More calls probably need this, but we can add it when we hit them.)
Test Plan: Looked at Diffusion as a logged-out user with public access enabled.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7380
Summary: `class_exists()` is case-insensitive, but `PhabricatorApplication::getByClass()` is not.
Test Plan: Fixed unit test to fail, then fixed code to pass unit test.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7379
Summary:
Ref T1344. This is //very// rough. Some UI issues:
- Empty states for the board and columns are junky.
- Column widths are crazy. I think we need to set them to fixed-width, since we may have an arbitrarily large number of columns?
- I don't think we have the header UI elements in M10 yet and that mock is pretty old, so I sort of very roughly approximated it.
- What should we do when you click a task title? Popping the whole task in a dialog is possible but needs a bunch of work to actually work. Might need to build "sheets" or something.
- Icons are slightly clipped for some reason.
- All the backend stuff is totally faked.
Generally, my plan is just to use these to implement all of T390. Specifically:
- "Kanban" projects will have "Backlog" on the left. You'll drag them toward the right as you make progress.
- "Milestone" projects will have "No Milestone" on the left, then "Milestone 9", "Milestone 8", etc.
- "Sprint" projects will have "Backlog" on the left, then "Sprint 31", "Sprint 30", etc.
So all of these things end up being pretty much exactly the same, with some minor text changes and new columns showing up on the left vs the right or whatever.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: chad, aran, sascha-egerer
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7374
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.
This has several parts:
- For PolicyAware queries, provide an application class name method.
- If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
- For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.
Test Plan:
- Added a unit test to verify I got all the class names right.
- Browsed around, logged in/out as a normal user with public policies on and off.
- Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7367
Summary:
Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions.
Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc.
Test Plan: Browsed several revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3415, T2222
Differential Revision: https://secure.phabricator.com/D7375
Summary: Ref T4010. Adds storage and indexes for custom fields. These tables are the same as people/maniphest/differential.
Test Plan: Ran `bin/storage upgrade`.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7369
Summary:
Fixes T3675.
- Maniphest had a couple of old non-event listeners; move them to events.
- Make most of the similar listeners a little more similar.
- Add checks for access to the application.
Test Plan:
- Viewed profile, project, task, revision.
- Clicked all the actions.
- Blocked access to various applications and verified the actions vanished.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3675
Differential Revision: https://secure.phabricator.com/D7365
Summary:
Ref T3675. Some of these listeners shouldn't do their thing if the viewer doesn't have access to an application (for example, users without access to Differential should not be able to "Edit Tasks"). Set the stage for that:
- Introduce `PhabricatorEventListener`, which has an application.
- Populate this for event listeners installed by applications.
- Rename the "PeopleMenu" listeners to "ActionMenu" listeners, which better describes their modern behavior.
This doesn't actually change any behaviors.
Test Plan: Viewed Maniphest, Differntial, People.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3675
Differential Revision: https://secure.phabricator.com/D7364
Summary: Ref T1279. The new stuff seems stable, so stop writes to the old tables.
Test Plan:
- Added and removed reviewers.
- Grepped for `::RELATIONSHIP_TABLE` to verify we really have no more reads.
- Grepped for `::RELATION_REVIEWER`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7360
Summary:
This is primarily a client request, and a little bit use-case specific, but policies seem to be holding up well and I'm getting more comfortable about maintaining this. Much if it can run through ApplicationTransactions.
Allow the ability to edit status, policies, priorities, assignees and projects of a task to be restricted to some subset of users. Also allow bulk edit to be locked. This affects the editor itself and the edit, view and list interfaces.
Test Plan: As a restricted user, created, edited and commented on tasks. Tried to drag them around.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7357
Summary: Fixes T4001. I broke this some time ago and no one has complained. I don't think it gets much use, and we haven't added it for the newer apps. Just get rid of it rather than adapt the URIs for ApplicationSearch.
Test Plan: Unit tests, sent myself some email.
Reviewers: zeeg, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4001
Differential Revision: https://secure.phabricator.com/D7355
Summary: Drop the "Pro" bit.
Test Plan: Created/edited tasks, moved tasks around, generally made a mess. Nothing burned down.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7352
Summary:
We have this commented-out chunk of code now which was originally buggy and is now just nonfunctional.
For now, the core edit types don't always require CAN_EDIT (e.g., subscribe, comment, add edges), except for editing the edit policy itself, which always does. Add a supplemental capability check there and let everything else go through with CAN_VIEW. We can buff the policy checks on application editors over time, they all require appropriate capabilities to get to in the first place anyway.
Test Plan: Created and edited some tasks without getting overwhelmed with policy exceptions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7351
Summary:
Ref T3569. Two issues:
# Since `sort()` is not stable, instantaneous events (ending on the same second they start) would sometime sort wrong and produce the wrong results. Guarantee they sort correctly.
# Because events can end at any time, there are some additional special cases the algorithm didn't handle properly. Draw a bunch of ASCII art diagrams so these cases work properly.
Test Plan:
- No more fatal when tracking an object for the first time.
- Unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: skyronic, aran
Maniphest Tasks: T3569
Differential Revision: https://secure.phabricator.com/D7350
Summary:
Ref T4007. Fixes T4009. Ref T4008.
- Move blurb to a text section.
- Make it render as remarkup.
- Put policy information and status information in header.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4009, T4007, T4008
Differential Revision: https://secure.phabricator.com/D7373
Summary:
I'll hold this for a couple weeks.
These classes are now only used to render legacy feed stories. I don't plan to migrate the stories since I don't think they're particularly valuable, and migrating them would be complex and time consuming.
With these classes removed, legacy Maniphest feed stories simply vanish from feed.
Test Plan: `grep`, viewed feed, verified it worked but omitted old-style stories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7114
Summary: Fixes T3898. This feature needs generalization at some point, but just unbreak it for now since a surprising number of users like it.
Test Plan: Pressed "z".
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran, spicyj
Maniphest Tasks: T3898
Differential Revision: https://secure.phabricator.com/D7366
Summary:
See screenshots. Some simplifications:
- Tabbed and non-tabbed lists are now allowed to be mixed. We just make the non-tabbed lists permanent and put them on the bottom (e.g., image and audio data in Files).
- You can provide a tab name instead of an entire tab object and we'll build an object for you.
- We respect `setSelected()` on the tab objects now.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7362
Summary: Make tabs do stuff when you click 'em.
Test Plan:
- Clicked object box tabs in UIExample.
- Viewed some existing non-tab UIs (Differential, Maniphest).
- Viewed some existing non-tab, multiple-list UIs (Diffusion).
- Grepped for methods I changed.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7361
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.
The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.
Two risks here:
- I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
- This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.
I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.
Test Plan:
- Before migrating, then after migrating:
- Made a bunch of inlines (drafts, submitted).
- Edited and deleted inlines.
- Verified inlines showed up in preview.
- Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
- Verified that inlines ARE indexed when they're not drafts.
- Verified that drafts inlines make revisions appear as "with draft" in the revision list.
- Made left, right, and draft inlines.
- Migrated (`bin/storage upgrade`).
- Verified that my inlines from before the migration still showed up.
- (Repeated all the stuff above.)
- Manually inspected the inline comment table.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7139
Summary: I touched this code recently but it needs an unusual special case because we call through with the "omnipotent user" from the daemons. As per the TODO below, this will all get cleaned up at some point.
Test Plan: Will make @poop verify.
Reviewers: btrahan, poop
Reviewed By: poop
CC: poop, aran
Differential Revision: https://secure.phabricator.com/D7356
Summary: This capability was replaced with an application-wide "manage" capability. It's checked for just above.
Test Plan: Edited audio!
Reviewers: btrahan, ljalonen, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7353
Summary:
Ref T3569. Fixes T3567. When figuring out how much time has been spent on an object, subtract "preemptive" events which interrupted the object.
Also, make the UI look vaguely sane:
{F72773}
Test Plan: Added a bunch of unit tests, mucked around in the UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, skyronic, aran
Maniphest Tasks: T3567, T3569
Differential Revision: https://secure.phabricator.com/D7349
Summary: Ref T603. While policies aren't completely perfect, they are substantially functional to the best of my knowledge -- definitely in good enough shape that we want to hear about issues with them, now.
Test Plan: Edited a task, repository, and project.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7343
Summary: Ref T603. If you don't have access to an application, prevent execution of its (authenticated) methods.
Test Plan: Restricted Tokens to only admins, then tried to view/call Token methods as a non-admin.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7342
Summary:
I refactored this recently and accidentally dropped the download URI.
Also fix a warning with, e.g., files named `README`.
Test Plan: Clicked a thumb, clicked "Download", got a file.
Reviewers: chad, btrahan, dctrwatson
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7341
Summary:
Ref T603. Currently, we render handles the user doesn't have permission to see in a manner identical to handles that don't exist. This is confusing, and not required by policies (which restrict content, but permit knowledge that an object exists).
Instead, render them in different styles. Bad/invalid objects look like:
Unknown Object (Task)
Restricted objects look like:
[o] Restricted Task
...where `[o]` is the padlock icon.
Test Plan:
{F71100}
{F71101}
It's possible this renders weird somewhere, but I wasn't immediately able to find any issues. Yell if you see something.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7334
Summary: Makes it easy to choose distinctive icons for projects.
Test Plan:
{F71018}
{F71020}
{F71019}
{F71021}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7333
Summary: Various tweaks and fixes. Adds a File Contents view in Diffusion, normalizes spaces, colors.
Test Plan: tested differential and diffusion in my sandbox.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3952
Differential Revision: https://secure.phabricator.com/D7325
Summary: See IRC. Someone got a `null` in CCPHIDs somehow. Moving to subscriptions should prevent this, but paper over it for now.
Test Plan: Will have @dctrwatson check.
Reviewers: btrahan, dctrwatson
Reviewed By: btrahan
CC: dctrwatson, aran
Differential Revision: https://secure.phabricator.com/D7330
Summary: Ref T603. Give countdowns proper UI-level policy controls, and an application-level default policy. Put policy information in the header.
Test Plan:
- Adjusted default policy.
- Created new countdowns.
- Edited countdowns.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7322
Summary: Ref T603. Gives the create/edit interface a policy control, and adds an application-level default.
Test Plan: Created and edited polls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7321
Summary:
Ref T603. Fixes T2823. This updates Paste and Macro.
- **Paste**
- Added default view policy.
- I didn't add a "create" policy, since I can't come up with any realistic scenario where you'd give users access to pastes but not let them create them.
- **Macro**
- Added a "manage" policy, which covers creating and editing macros. This lets an install only allow "People With An Approved Sense of Humor" or whatever to create macros.
- Removed the "edit" policy, since giving individual users access to specific macros doesn't make much sense to me.
- Changed the view policy to the "most public" policy the install allows.
- Added view policy information to the header.
Also fix a couple of minor things in Maniphest.
Test Plan:
- Set Paste policy, created pastes via web and Conduit, saw they got the right default policies.
- Set Macro policy, tried to create/edit macros with valid and unauthorized users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2823, T603
Differential Revision: https://secure.phabricator.com/D7317
Summary: Adds filetype icons, applying to differential file headers. The main issue is with all the lightening, I wanted something to still anchor 'new file' on the page and adding a sharp icons does that pretty well for me. Feedback is cool too.
Test Plan: Add some new icons, test in previous commits.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7320
Summary: The "user" and "user/project" sources exclude system agents and disabled users, but should not.
Test Plan: Added system agents to Herald rules.
Reviewers: btrahan, bigo
Reviewed By: bigo
CC: aran
Differential Revision: https://secure.phabricator.com/D7319
Summary: Ref T603. When a user selects "Custom", we pop open the rules dialog and let them create a new rule or edit the existing rule.
Test Plan: Set some objects to have custom policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7300
Summary:
Instead of returning a blank result it throws exceptions. Fix this up a
little so we get some consistency with differential
Test Plan:
Loaded a bad phid for releeph, returns empty list.
Try a good phid and get 2 releeph merges.
Reviewers: epriestley, elenaperezrioja, dschleimer, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7302
Summary:
Ref T603. Several issues here:
1. Currently, `FileQuery` does not actually respect object attachment edges when doing policy checks. Everything else works fine, but this was missing an `array_keys()`.
2. Once that's fixed, we hit a bunch of recursion issues. For example, when loading a User we load the profile picture, and then that loads the User, and that loads the profile picture, etc.
3. Introduce a "Query Workspace", which holds objects we know we've loaded and know we can see but haven't finished filtering and/or attaching data to. This allows subqueries to look up objects instead of querying for them.
- We can probably generalize this a bit to make a few other queries more efficient. Pholio currently has a similar (but less general) "mock cache". However, it's keyed by ID instead of PHID so it's not easy to reuse this right now.
This is a bit complex for the problem being solved, but I think it's the cleanest approach and I believe the primitive will be useful in the future.
Test Plan: Looked at pastes, macros, mocks and projects as a logged-in and logged-out user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7309
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.
Some policy objects don't have real PHIDs:
PhabricatorTokenGiven
PhabricatorSavedQuery
PhabricatorNamedQuery
PhrequentUserTime
PhabricatorFlag
PhabricatorDaemonLog
PhabricatorConduitMethodCallLog
ConduitAPIMethod
PhabricatorChatLogEvent
PhabricatorChatLogChannel
Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.
Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.
Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7306
Summary: Ref T3958. Adds a provider for Mozilla's Persona auth.
Test Plan:
- Created a Persona provider.
- Registered a new account with Persona.
- Logged in with Persona.
- Linked an account with Persona.
- Dissolved an account link with Persona.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3958
Differential Revision: https://secure.phabricator.com/D7313
Summary:
Ref T603. Although I think the parenthetical is valuable when //setting// policies to make sure no one accidentally opens content up, it's super annoying in headers.
This makes headers say "Public". Everything else still says "Public (No Login Required)".
Test Plan: {F69469}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7310
Summary:
also try to centralize some of the command parsing logic. note that differential is still an exception here. it uses a whitelist-style regex. i think long-term we should have this for every app but changing it seemed too big for this diff.
Fixes T3937.
Test Plan:
echo '!assign btrahan' | ./bin/mail receive-test --as xerxes --to T22 ; echo '!claim' | ./bin/mail receive-test --as xerxes --to T22
unit tests passed, though my new one is silly
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3937
Differential Revision: https://secure.phabricator.com/D7307
Summary: Ref T603. Allow the endpoint to take an existing policy PHID to populate the editor and return a useful datastructure.
Test Plan: In the next revision, actually hooked this up.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7299
Summary:
Ref T603. Make these actually implement policy interfaces, so shared infrastructure (like handle loading) works as expected. They don't actually have meaningful policies, and we short circuit all the checks.
(I don't plan to let you set policy controls on policies themselves)
Test Plan: Loaded handles for Policy objects via common infrastructure.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7298
Summary: Ref T603. Fix/provide some rendering stuff related to custom policies.
Test Plan: After setting stuff to custom policies (made easier by future diffs), looked at the various places strings appear in the UI and saw more sensible ones.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7297
Summary: Ref T603. This is "Allow" in the UI, I just mistyped it when I created the constant.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7296
Summary: Ref T603. Adds code to actually execute custom policies. (There's still no way to select them in the UI.)
Test Plan:
- Added and executed unit tests.
- Edited policies in existing applications.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7292
Summary: Fixes T3950. This centers the images, adds a thin blue border, and a transparent background.
Test Plan: Tested a file in Files, Diffusion, and Macro.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3950
Differential Revision: https://secure.phabricator.com/D7305
Summary: This adds some controllable space between paths in Diffusion headers. Fixes T3951
Test Plan: Tested new links in diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3951
Differential Revision: https://secure.phabricator.com/D7304
Summary: Ref T603. After thinking about this for a bit I can't really come up with anything better than what Facebook does, so I'm going to implement something similar for choosing custom policies. To start with, swap this over to a JS-driven dropdown.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7285
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.
Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7283
Summary:
we filter the $actors above such that its possible to have no $actor anymore (if $actor is not a deliverable email address). ergo, make sure we have actor before we start calling methods.
Fixes github issue 403
Test Plan: logic on this one - not 100% sure how to easily reproduce
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7284
Summary:
- Add an extra paginator at the top.
- Add a link to jump to the bottom (where the latest messages are).
- Align paginators with edge of content rather than the page.
Test Plan: Looked at the chatlog.
Reviewers: epriestley, chad, #blessed_reviewers
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7280
Summary: UX on this could probably be better 'disabled' crumbs don't appear to have any visible difference, and the policy error has to load the /create page rather than being a modal - not sure on the way to fix these.
Test Plan: Tried to create a project with and without access, saw suitable error.
Reviewers: epriestley, #blessed_reviewers
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7279
Summary: Ref T603. I nuked this check by accident and neglected to test the negative case.
Test Plan: Saved a non-public policy (Herald Global) and a public policy (Maniphest View).
Reviewers: asherkin, btrahan
Reviewed By: asherkin
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7278
Test Plan: Looked at Home with Audit installed and uninstalled.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7277
Summary: Ref T603. Currently, we hard-code defense against setting policies to "Public" in several places, and special case only the CAN_VIEW policy. In fact, other policies (like Default View) should also be able to be set to public. Instead of hard-coding this, move it to the capability definitions.
Test Plan: Set default view policy in Maniphest to "Public", created a task, verified default policy.
Reviewers: btrahan, asherkin
Reviewed By: asherkin
CC: asherkin, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7276
Summary:
Ref T603. This isn't remotely usable yet, but I wanted to get any feedback before I build it out anymore.
I think this is a reasonable interface for defining custom policies? It's basically similar to Herald, although it's a bit simpler.
I imagine users will rarely interact with this, but this will service the high end of policy complexity (and allow the definition of things like "is member of LDAP group" or whatever).
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, asherkin
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7217
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.
I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.
Test Plan: Created new revisions from the CLI and conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7269
Summary:
Ref T603. In thinking about this, I think I went mad with power in creating this capability. I can't imagine any reason to give users access to Herald but not let them create rules.
We can restore this later if some install comes up with a good reason to have it, but in the interest of keeping policies as simple as possible, I think we're better off without it. In particular, if you don't want a group of users creating rules, just lock them out of the application entirely.
The "Manage Global Rules" capability is still around, I think that one's super good.
Test Plan: Edited Herald policies, created a rule.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7268
Summary: Ref T603. Allow global default policies to be configured for tasks.
Test Plan:
- Created task via web UI.
- Created task via Conduit.
- Created task via email.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7267
Summary: Ref T603. When the user encounters an action which is controlled by a special policy rule in the application, make it easier for applications to show the user what policy controls the action and what the setting is. I took this about halfway before and left a TODO, but turn it into something more useful.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7265
Summary: Ref T603. Use more modern elements.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7264
Summary: Ref T603. Use the new hotness.
Test Plan: Edited Herald in Applications, tried to create rules / global rules without capabilities, got reasonable error messages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7263
Summary: Adds the abilit to set a status color of warning or fail to navbar tab lists (for objectheaders)
Test Plan: uiexamples, photoshop
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7266
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.
Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.
That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.
Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.
Also provide more context, and more useful exception messages.
Test Plan:
- See screenshots.
- Also triggered a policy exception and verified it was dramatically more useful than it used to be.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7260
Summary: Ref T603. Apparently we made all policies possible at some point. Go us! This has no callsites.
Test Plan: `grep`, notice it's a private method
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7259
Summary: Ref T1279. Prerequisite for adding icons or other type information to tokenizers, since we don't currently have enough information to prefill them when rendering things from the server side. By passing handles in, the tokenizer can extract type information.
Test Plan:
- Searched by user in Audit.
- Sent Conpherence from profile page.
- Tried to send an empty conpherence.
- Searched Countdown by user.
- Edited CCs in Differential.
- Edited reviewers in Differential.
- Edited a commit's projects.
- Searched lint by owner.
- Searched feed by owner/project.
- Searched files by owner.
- Searched Herald by owner.
- Searched Legalpad by owner.
- Searched Macro by owner.
- Filtered Maniphest reports by project.
- Edited CCs in Maniphest.
- Searched Owners by owner.
- Edited an Owners package.
- Searched Paste by owner.
- Searched activity logs by owner.
- Searched for mocks by owner.
- Edited a mock's CCs.
- Searched Ponder by owner.
- Searched projects by owner.
- Edited a Releeph project's pushers.
- Searched Releeph by requestor.
- Edited "Uses Symbols" for an Arcanist project.
- Edited all tokenizers in main search.
- Searched Slowvote by user.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7248
Summary: Ref T603. We currently bomb out here, but should just continue forward. I'm fairly certain we don't even use this for anything anymore (it has been replaced by "depends on") but need to check that.
Test Plan: Created a new revision with `arc diff`.
Reviewers: ljalonen, btrahan, #blessed_reviewers, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7255
Summary: Ref T1279. I only tested the global case. :O
Test Plan: Created a personal "add me as blocking" rule.
Reviewers: btrahan, zeeg
Reviewed By: zeeg
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7261
Summary:
Ref T603. Ref T1279. Further improves transaction and policy support for Herald.
- Instead of deleting rules (which wipes out history and can't be undone) allow them to be disabled.
- Track disables with transactions.
- Gate disables with policy controls.
- Show policy and status information in the headers.
- Show transaction history on rule detail screens.
- Remove the delete controller.
- Support disabled queries in the ApplicationSearch.
Test Plan:
- Enabled and disabled rules.
- Searched for enabled/disabled rules.
- Verified disabled rules don't activate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279, T603
Differential Revision: https://secure.phabricator.com/D7247
Summary:
Ref T1279. This is a logical change.
- "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
- "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".
Practically, the primary net effect of this is just to make blocking reviews actually block.
This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.
Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7245
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.
Test Plan: Added Herald rules and updated revisions; see screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7244
Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.
This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.
Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7243
Summary: Ref T1279. If you accept a revision, also accept on behalf of all the projects you have authority to accept for.
Test Plan:
- Accepted a revision which I was a reviewer on, saw my own status and an authority project's status change to "Accepted".
- Accepted a revision which I was not a reviewer on, saw my own status be added (as "Accepted") and the project's status update.
Also, see screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7242
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:
- It puts queries very close to the display layer.
- We have to query for each revision if we want to figure out authority for several.
- We need to figure it out in several places, so we'll end up with copies of this logic.
- The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
- We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.
Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.
Test Plan:
- Looked at some revisions, verified the correct lines were highlighted.
- Looked at a revision I created and verified that projects I was a member of were not highlighted.
- With self-accept enabled, these //are// highlighted.
- Looked at a revision I did not create and verified that projects I was a member of were highlighted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7241
Summary: Ref T603. Fixes T3921. Tightens up policy controls for file/object relationships in existing applications.
Test Plan:
- Uploaded new project image, verified it got an edge to the project.
- Uploaded new profile image, verified it got an edge to me.
- Uploaded new macro image, verified it got an edge to the macro.
- Uploaded new paste via web UI and conduit, verified it got attached.
- Replaced, added images to a mock, verified they got edges.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3921, T603
Differential Revision: https://secure.phabricator.com/D7254
Summary:
Ref T603. Move toward stamping out all the Project / ProjectProfile query irregularities with respect to policies.
- Fixes a bug with Asana publishing when the remote task is deleted.
- Fixes an issue with Herald commit rules.
Test Plan:
- Viewed projects;
- edited projects;
- added and removed members from projects;
- republished Asana-bridged feed stories about commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7251
Summary:
Ref T1279. This allows installs to implement two different flavors of project review. They can either implement this rule:
When:
[ ... ] [ ... ]
Take Action:
[ Add blockign reviewers ] [ Security ]
...which means "every revision matching X needs to be signed off by someone else on the Security team, //even if the author is on that team//". The alternative is to implement this rule:
When:
[ Author's projects ] [ do not include ] [ Security ]
[ ... ] [ ... ]
Take Action:
[ Add blocking reviewers ] [ Security ]
...which means that people on the Security team don't need a separate signoff from someone else on the team.
I think this weaker version maps to some of what, e.g., Google does (you need to be reviewed by someone with "readability" in a language, but if you have it that's good enough), but I could imagine cases like "Security" wanting to prevent self-review from satisfying the requirement.
@zeeg, not sure which of these use cases is relevant here, but either one should work after this.
Test Plan: Created rules with this field, verified it populated properly in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7238
Summary: Ref T1279. No logical changes, but cosmetically highlight stuff you have authority for, like we do in Diffusion.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7237
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.
I will probably write some lengthy treatise on how you shouldn't use this rule later.
Implementation is straightforward because Differential previously supported this rule.
This rule can also be used to add project reviewers.
Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7235
Summary: Ref T1279. Show separate sections for "Reviewers" and "Project Reviewers" (Differential) and for "Auditors" and "Package/Project Auditors" (Diffusion/Audit).
Test Plan:
- Looked at a commit. Saw separation.
- Looked at a revision. Saw separation.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7233
Summary:
Ref T1279. Two changes to the search/query for Differential:
- "Reviewers" now accepts users and projects.
- "Responsible Users" now includes revisions where a project you are a member of is a reviewer.
Test Plan:
- Searched for project reviewers.
- Verified that the dashboard now shows reviews which I'm only part of via project membership.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7231
Summary:
Ref T1279. No actual logical changes, but:
- You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
- You can now add projects as reviewers from the revision detail typeahead.
- You can now add projects as reviewers from the CLI (`#yoloswag`).
- Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).
I'll separate projects from users in the "Reviewers" tables in the next revision.
Test Plan:
- Added projects as reviewers using the web UI and CLI.
- Used `arc amend --show --revision Dnnn` to generate commit messages.
- Viewed revision with project reviewers in web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7230
Summary: Ref T1279. Updates status to 'accepted' or 'commented' when the user takes those actions.
Test Plan:
- Commented on a revision, got a comment icon.
- Accepted a revision, got an accept icon.
- Commented again, icon stayed as "accept".
- Faked the "old diff" states.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7229
Summary:
Ref T1279. No logical changes, just updates the reviewer display style.
We currently keep track of only "requested changes".
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7228
Summary:
Ref T1279. @champo did a lot of this work already; we've been doing double writes for a long time.
Add "double reads" (reading the edge table as both the "relationship" table and as the "reviewer status" table), and migrate all the data.
I'm not bothering to try to recover old reviewer status (e.g., we could infer from transactions who accepted old revisions) because it wold be very complicated and doesn't seem too valuable.
Test Plan:
- Without doing the migration, used Differential. Verified that reads and writes worked. Most of the data was there anyway since we've been double-writing.
- Performed the migration. Verified that everything was still unchanged.
- Dropped the edge table, verified all reviweer data vanished.
- Migrated again, verified the reviewer stuff was restored.
- Did various cc/reviewer/subscriber queries, got consistent results.
Reviewers: btrahan
Reviewed By: btrahan
CC: champo, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7227
Summary:
Ref T603. This closes the other major policy loophole in Herald, which was that you could write a rule like:
When [Always], [Add me to CC]
...and end up getting email about everything. These rules are now enforced:
- For a //personal// rule to trigger, you must be able to see the object, and you must be able to use the application the object exists in.
- In contrast, //global// rules will //always// trigger.
Also fixes some small bugs:
- Policy control access to thumbnails was overly restrictive.
- The Pholio and Maniphest Herald rules applied only the //last// "Add CC" or "Add Project" rules, since each rule overwrote previous rules.
Test Plan:
- Created "always cc me" herald and maniphest rules with a normal user.
- Created task with "user" visibility, saw CC.
- Created task with "no one" visibility, saw no CC and error message in transcript ("user can't see the object").
- Restricted Maniphest to administrators and created a task with "user" visibility. Same deal.
- Created "user" and "no one" mocks and saw CC and no CC, respectively.
- Thumbnail in Pholio worked properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7224
Summary:
Used `DifferentialRevisionQuery` with the relevant `need*()` calls in the test controller.
And started assuming the revision has reviewers and CC phids in `HeraldDifferentialRevisionAdapter`.
Test Plan:
Added herald rules that use revisions (one for revisions another for commit) and reviewers.
Created, accepted and landed a revision that matched the rules and checked all rules were applied.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6468
Conflicts:
src/applications/herald/adapter/HeraldCommitAdapter.php
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
src/applications/herald/controller/HeraldTestConsoleController.php
Summary:
maniphest tasks were fataling with priority 0 before making sure to add the return null if new object trick to the maniphest pro editor.
pholio had a problem where if you had no jpegs you were walking off array_rand. tighten the math and then just return a built-in if no uploaded user images could be found. Fixes T3889.
Test Plan: bin/lipsum generate for a few minutes and no errors
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3889
Differential Revision: https://secure.phabricator.com/D7222
Summary:
Ref T603. Herald transcripts potentially leak a bunch of content (task text, revision/commit content). Don't let users see them if they can't see the actual objects.
This is a little messy but ends up mostly reasonable-ish.
Test Plan:
- Verified that transcripts for objects I couldn't see no longer appear in the list, and reject access.
- Verified that transcripts for objects in applications I can't see reject access, albeit less gracefully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7221
Summary:
- Use the box view in the test console.
- Let the test console load tasks and mocks. We should move this to the adapters (`canAdaptObject($object)` or something).
- Fix a minor issue with "Always": hiding the whole cell could make the table layout weird in Safari, at least. Just hide the select instead.
Test Plan:
- Used test console on task.
- Used test console on mock.
- Created (silly) rule with "Always" and also some other conditions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7220
Summary: Adds an ObjectBox to Phabricator Registration
Test Plan: check logged out page for new header.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7223
Summary:
Ref T603. Herald is a bit of a policy minefield right now, although I think pretty much everything has straightforward solutions. This change:
- Introduces "create" and "create global" permisions for Herald.
- Maybe "create" is sort of redundant since there's no reason to have access to the application if not creating rules, but I think this won't be the case for most applications, so having an explicit "create" permission is more consistent.
- Add some application policy helper functions.
- Improve rendering a bit -- I think we probably need to build some `PolicyType` class, similar to `PHIDType`, to really get this right.
- Don't let users who can't use application X create Herald rules for application X.
- Remove Maniphest/Pholio rules when those applications are not installed.
Test Plan:
- Restricted access to Maniphest and uninstalled Pholio.
- Verified Pholio rules no longer appear for anyone.
- Verified Maniphest ruls no longer appear for restricted users.
- Verified users without CREATE_GLOBAL can not create global ruls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7219
Summary: Ref T603. This could be a nicer UX, but limit the amount of foot-shooting that users can possibly do. You can still manage if you're really tricky ("Members of project X", then leave the project) but this should make it hard to make a mistake. It seems very unlikely any user ever intends to lock themselves out of an application.
Test Plan: Set an application's view policy to permissive ("Administrators") and nonpermissive ("No One") values. The former were accepted, the latter rejected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7218
Summary: Right now emails don't include bookmark info (wasn't added in D2897). Lets include it so it's consistent with the web UI.
Test Plan: Inspected code, made sure it matched web UI code. Verified that web UI with these changes was consistent with rendering before refactoring.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7215
Summary:
Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'.
- There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant.
- The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally.
Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3829
Differential Revision: https://secure.phabricator.com/D7214
Summary:
Ref T603. I had to partially revert this earlier because it accidentally blocked access to Conduit and File data for installs without "policy.allow-public", since the applications are available to "all users" but some endpoints actually need to be available even when not logged in.
This readjusts the gating in the controller to properly apply application visibility restrictions, and then adds a giant pile of unit test coverage to make sure it sticks and all the weird cases are covered.
Test Plan:
- Added and executed unit tests.
- Executed most of the tests manually, by using logged in / admin / public / disabled users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7211
Summary:
Fixes T1461.
Adds
- FIELD_ALWAYS - now you could add this to a content type to always get notified
- FIELD_REPOSITORY_AUTOCLOSE_BRANCH - solves T1461
- CONDITION_UNCONDITIONALLY - used by these two fields to not show any value for the user to select
Test Plan: made a herald rule where diffs on autoclose branches would get flagged blue. made a diff on an autoclose branch and committed it. commit was flagged!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1461
Differential Revision: https://secure.phabricator.com/D7210
Summary:
In most cases this just makes the URIs more consistent, but it's funky/breakish for SVN repositories which are only partially tracked.
See also T3915, and IRC.
Test Plan:
- Browsed some repositories, verified URIs generated as expected, with trailing slashes for directories.
- Verified nothing goofy happened in the extremes (like double slashes on the first crumb).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7209
Summary: Ref T603. If you get in trouble, `bin/policy unlock PHID-APPS-PhabricatorApplicationDifferential` and such can get you out now.
Test Plan: Unlocked an application.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7206
Summary:
Ref T603. Enables:
- Application policies can be edited.
- Applications can define custom policies (this will be used for setting defaults, like "what is the default visibiltiy of new tasks", and meta-policies, like "who can create a task?").
Test Plan: Edited application policies. A future diff does more with custom policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7205
Summary:
Ref T603. Broadly, this allows you to implement a policy like "Only users in Engineering can use Differential."
This isn't complete, and there will be a long tail of special cases to deal with. Some examples:
- If you can't use Differential, should you still be able to attach/detach revisions from tasks?
- You currently will be able to.
- This actually seems pretty reasonable.
- But in other cases it might not be: the "send user a message" action should probably require access to Conpherence.
- If you can't use Differential, should you still be able to see feed stories about it?
- You currently will be able to, if you can see the revisions.
- This seems not-so-reasonable and we should probably lock it down.
- If you can't use Differential, can users CC you on revisions?
- Currently, they can, and you can't do anything about it.
- Probably they shouldn't be able to? This seems challenging to explain in the UI.
- If you can't use Differential, can you write a Herald rule against it?
- You currently will be able to.
- Seems like you obviously shouldn't be able to.
- I think this is a general issue right now (you can still write Differential herald rules even if you uninstall the application, I believe).
There are probably a few more things I haven't thought of. However, there are a finite number of these things and I suspect there aren't //too/ many more than this -- I can't come up with like 100 of them, and half of the ones above have easy fixes.
Despite the rough edges, I think this accomplishes 95% of what installs expect from it.
Test Plan: Restricted Differential and saw it vanish from the home page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7203
Summary:
I'm just going to store application policy settings (like view/edit policy, and default policies for content) in config, because:
1) We'll need access to it on every page, and Config is "free" since we already pull it.
2) Building separate storage and transactions seems like overkill, we get less-nice but pretty-reasonable transactions for free with config.
3) We could easily move it later if this is a bad call.
Also fix some formatting.
Test Plan: See future revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7202
Summary: Make the application query a little more flexible, and formalize the PHID type.
Test Plan: See next diffs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7201
Test Plan: Confirm the API returns a single flat result with a unified git diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, charles
Differential Revision: https://secure.phabricator.com/D7199
Test Plan:
Enable inline patches:
```
bin/config set metamta.differential.patch-format 'unified'
bin/config set metamta.differential.inline-patches 100000000
```
Create a new diff and confirm it renders correctly via email.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7198
Summary: Cleans up jump nav so it doesn't hard code a bunch of application behaviors. It still hard-codes a few, but few//er//?
Test Plan: Jumped to stuff like `D12`, `d`, `@dog`, `p admins only`, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7196
Summary: Ref T603. This didn't impact policies anyway, but using PhabricatorObjectQuery is far simpler and more general.
Test Plan: Used "Attach" dialog to find mocks, tasks, and revisions by "Dxx", "Mxx", etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7195
Summary: I removed the only callsite in D7179, but forgot to remove this code.
Test Plan: Grepped for callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7194
Summary: Ref T603. Make this rule properly policy-aware, and extend from `PhabricatorRemarkupRuleObject`.
Test Plan:
- Embedded an image, tested all options (name, link, float, layout, size).
- Used lightbox to view several images.
- Embedded a text file, tested all options (name).
- Embedded audio, tested all options (loop, autoplay).
- Attached a file via comment to a task, verified edge was created.
- Attached a file via comment to a conpherence, verified edge was created.
- Viewed old files, verified remarkup version bump rendered them correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7192
Summary: Ref T603. When a user comments on an object with an embedded file, write an "attached" edge.
Test Plan: Made a comment on a task with an embedded file, verified the edge was written in Files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7191
Summary: Ref T603. We might need a fine-grained CLI tool later on, but here's a bat we can bludgeon things with.
Test Plan:
- Ran `bin/policy unlock D12` (adjusted policies).
- Ran `bin/policy unlock rPca85c457ebcb` (got "not mutable" stuff).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7189
Summary:
I use color to convey meaning like "good resource to keep handy for a bit on new way of doing things" or "snipe this task". Now the list can be grouped by these colors.
Note I do this in PHP 'cuz color isn't part of any index AFAIK and pragmatically speaking this dataset should be tiny in the context of "user flags".
Ref T1809
Test Plan: selected group by color and observed the flags were indeed grouped by color
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D7188
Summary: Depends on D7163. This adds a "Stop Tracking" link to the right-hand side of ongoing entries in the Phrequent search view. It allows users to stop tracking items without first navigating to the item itself.
Test Plan: Started tracking and item and then clicked the "Stop Tracking" link in Phrequent.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3870
Differential Revision: https://secure.phabricator.com/D7164
Summary:
This updates Phrequent to use new the search infrastructure. Now it looks like:
{F60141}
I've also added the policy infrastructure stubs, but it's probably not even close to being right in terms of enforcing policies (in particular being able to see time tracked against objects the user wouldn't normally be able to see).
At some point I'd like to be able to filter on the objects that the time is tracked against, but I don't believe there's a tokenizer / readahead control that allows you to type any kind of object.
Test Plan: Clicked around the new interface, created some custom queries and saved them.
Reviewers: epriestley
CC: Korvin, aran
Maniphest Tasks: T3870
Differential Revision: https://secure.phabricator.com/D7163
Summary:
Ref T3903. Ref T603. We currently overreact to invalid policies. Instead:
- For non-omnipotent users, just reject the viewer.
- For omnipotent users, we already shortcircuit and permit the viewer.
- Formalize and add test coverage for these behaviors.
Also clean up some strings.
The practical effect of this is that setting an object to an invalid policy (either intentionally or accidentally) doesn't break callers who are querying it.
Test Plan:
- Created a Legalpad document and set view policy to "asldkfnaslkdfna".
- Verified this policy behaved as though it were "no one".
- Added, executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T3903
Differential Revision: https://secure.phabricator.com/D7185
Summary:
Currently, if you attach a revision to a task and the revision has a title with quotes or angle brackets in it, they are over-escaped in the email.
Instead, don't do that.
Test Plan: Attached `"QUOTES" MATH: 1 < 2` to a task, got a reasonable looking email.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7186
Summary:
Ref T603. Principally, I want to implement the rule "when you upload a file to an object, users must be able to see the object in order to see the file", since I think this is strongly in line with user expectation. For example, if you attach a file to a Conpherence, it should only be visible to members of that thread.
This adds storage for policies, but doesn't do anything interesting with it yet.
Test Plan: Ran `bin/storage upgrade`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7175
Summary:
Ref T603. This uses the existing edges (from Conpherence) to record that a file is attached to an object, and uses those edges to create a policy exception: if you can view an attached object, you can view a file.
I'm going to combine this with restrictive defaults to satisfy the other half of the equation (that files you attach to a conpherence usually shouldn't be public by default).
Test Plan:
- Loaded `/files/`.
- Uploaded a file to a Conpherence, looked at it in Files, saw the attachment.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7182
Summary:
- "revision" is misspelled.
- Remove an unused variable.
Test Plan: Used API console to call method.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7184
Summary: See D7162. This was like 99% my fault. Just provide a header; the new ones look pretty reasonable.
Test Plan: Viewed Diffusion change view, no exception.
Reviewers: vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7183
Summary: Ref T603. This has some custom logic which ObjectQuery can now perform more simply and more correctly.
Test Plan: Ran `bin/files purge F1`, `bin/files purge D1`, `bin/files purge --all`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7180
Summary: Ref T603. Clean these up and move them to a single place.
Test Plan:
- Downloaded a raw diff.
- Enabled "attach diffs", created a revision, got an email with a diff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7179
Summary: Ref T603. Swaps out most `PhabricatorFile` loads for `PhabricatorFileQuery`.
Test Plan:
- Viewed Differential changesets.
- Used `file.info`.
- Used `file.download`.
- Viewed a file.
- Deleted a file.
- Used `/Fnnnn` to access a file.
- Uploaded an image, verified a thumbnail generated.
- Created and edited a macro.
- Added a meme.
- Did old-school attach-a-file-to-a-task.
- Viewed a paste.
- Viewed a mock.
- Embedded a mock.
- Profiled a page.
- Parsed a commit with image files linked to a revision with image files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7178
Summary:
Fixes T3894. The "Log Out" icon has moved away from its rightmost position in the menubar.
In rP2e5ac12, I added a "Policy" application. This was the root cause.
The reordering logic (below) is slightly wrong. The `array_select_keys()` call is actually using the //strings// (like "Admnistration") to select the groups, not the correct constants (like "admin"). Use the constants instead and get the expected group ordering.
Test Plan: Loaded page, "Log Out" is in the rightmost position.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3894
Differential Revision: https://secure.phabricator.com/D7177
Summary:
Three changes here.
- Add `setActionList()`, and use that to set the action list.
- Add `setPropertyList()`, and use that to set the property list.
These will let us add some apropriate CSS so we can fix the border issue, and get rid of a bunch of goofy `.x + .y` selectors.
- Replace `addContent()` with `appendChild()`.
This is just a consistency thing; `AphrontView` already provides `appendChild()`, and `addContent()` did the same thing.
Test Plan:
- Viewed "All Config".
- Viewed a countdown.
- Viewed a revision (add comment, change list, table of contents, comment, local commits, open revisions affecting these files, update history).
- Viewed Diffusion (browse, change, history, repository, lint).
- Viewed Drydock (resource, lease).
- Viewed Files.
- Viewed Herald.
- Viewed Legalpad.
- Viewed macro (edit, edit audio, view).
- Viewed Maniphest.
- Viewed Applications.
- Viewed Paste.
- Viewed People.
- Viewed Phulux.
- Viewed Pholio.
- Viewed Phame (blog, post).
- Viewed Phortune (account, product).
- Viewed Ponder (questions, answers, comments).
- Viewed Releeph.
- Viewed Projects.
- Viewed Slowvote.
NOTE: Images in Files aren't on a black background anymore -- I assume that's on purpose?
NOTE: Some jankiness in Phortune, I'll clean that up when I get back to it. Not related to this diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7174
Summary: Fixes 2x white icons, adds 'user' and 'project' icons.
Test Plan: tested new states in Maniphest
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7176
Summary:
Ref T603. I want to provide at least a basic CLI tool for fixing policy problems, since there are various ways users can lock themselves out of objects right now. Although I imagine we'll solve most of them in the application eventually, having a workaround in the meantime will probably make support a lot easier.
This implements `bin/policy show <object>`, which shows an object's policy settings. In a future diff, I'll implement something like `bin/policy set --capability view --policy users <object>`, although maybe just `bin/policy unlock <object>` (which sets view and edit to "all users") would be better for now. Whichever way we go, it will be some blanket answer to people showing up in IRC having locked themselves out of objects which unblocks them while we work on preventing the issue in the first place.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7171
Summary: Missed this case in my sandbox
Test Plan: Reload a test diff
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7168
Summary: This adds the 'PHUIObjectBox' to nearly every place that should get it. I need to comb through Diffusion a little more. I've left Differential mostly alone, but may decide to do it anyways this weekend. I'm sure I missed something else, but these are easy enough to update.
Test Plan: tested each new layout.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7162
Summary:
Fixes T3887. Two issues:
- Macros were generating entirely before the render cache, so audio macros worked fine in previews and the first time the cache was populated, but not afterward.
- Instead, parse them before the cache but drop them in after the cache. Clean up all the file querying, too. This makes cached remarkup generate the correct audio beahviors.
- Safari sends an HTTP request with a "Range" header, and expects a "206 Partial Content" response. If we don't give it one, it sometimes has trouble figuring out how long a piece of audio is (mostly for longer clips? Or mostly for MP3s?). I'm not exactly sure what triggers it. The net effect is that "loop" does not work when Safari gets confused. While looping a short "quack.wav" worked fine, longer MP3s didn't loop.
- Supporting "Range" and "206 Partial Content", which is straightforward, fixes this problem.
Test Plan:
- Viewed a page with lots of different cached audio macros and lots of different uncached preview audio macros, they all rendered correctly and played audio.
- Viewed a macro with a long MP3 audio loop in Safari. Verified it looped after it completed. Used Charles to check that the server received and responded to the "Range" header correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7166
Summary: Fixes T3883. This is already supported in the query, expose it in the UI.
Test Plan: Ran some queries with and without subscribers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3883
Differential Revision: https://secure.phabricator.com/D7161
Summary:
Fixes T3887. Basically:
- Macros with audio get passed to the `audio-source` behavior.
- This keeps track of where they are relative to the viewport as the user scrolls.
- When the user scrolls a "once" macro into view, and it reaches roughly the middle of the screen, we play the sound.
- When the user scrolls near a "loop" macro, we start playing the sound at low volume and increase the volume as the user scrolls.
This feels pretty good on both counts.
Test Plan: Tested in Safari, Chrome, and Firefox. FF seems a bit less responsive and doesn't support MP3, but it was fairly nice in Chrome/Safari.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7160
Summary: Ref T3887. Implements storage and editors, but not the actual audio part.
Test Plan: Edited audio, audio behaviors of macros. Transactions and email looked good. Hit error cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7159
Summary: we were bad at displaying phid-based values nicely. Now we are good at it.
Test Plan: made a herald rule where if the author was a or b, the task should be assigned to c and have projects x, y, z added to it. this displayed nicely.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7158
Summary: Ref T3887. Similar to how we render images with `<img />`, render audio with `<audio />` if possible.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7156
Summary: Ref T603. We have a real policy app now, so put the config options there. Revise the description of the public policy switch to make it clear that enabling it immediately opens up the user directory and various other interfaces.
Test Plan: Viewed/edited config setting.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7154
Summary:
Ref T603. If an install allows acccess by logged-out users, show search.
(A lot of the search typeahead results, although visible to the user, don't lead anywhere interesting right now. We can clean this up in the future.)
Test Plan: As a logged out user, searched for some stuff. It worked. Also, I only found results I could see, which is quite heartening.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7153
Summary:
Ref T603. I got most of this earlier, but finish it up.
- Make a couple of controllers public; pretty much everything in Diffusion has implicit policy checks as a result of building a `DiffusionRequest`.
- Add an "Edit" capability to commits.
- Swap out the comment thing for commits.
- Disable actions if the user can't take them.
Test Plan: Viewed a bunch of interfaces while logged out, got appropriate results or roadblocks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7152
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.
- Provide as much detail as possible.
- Fix all the strings for i18n.
- Explain special rules to the user.
- Allow indirect policy filters to raise policy exceptions instead of 404s.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7151
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".
This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7150
Summary:
Ref T603. Make Differential behaviors for logged-out and underprivleged users more similar to other apps.
I'm going to drop this "anonymous access" thing at some point, but `reviews.fb.net` actually looks like it's running semi-modern code, so leave it alive until we have a more compelling replacement in the upstream.
Test Plan: As a logged out user, browsed Differential and clicked things and such.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7148
Summary: right now you get sent an email with a broken link 'cuz the email is plain text if you edit something with the edit policy being a project.
Test Plan: edited a legalpad document edit policy repeatedly to various projects. observed good emails via bin/mail debug tool. object page still looked good too
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7149
Summary:
Ref T603. When a diff is attached to a revision, try to guess the repository if possible. In cases where we succeed, this automatically gives us intuitive policy behavior (i.e., you can see a revision if you can see the repository the change is against).
I pulled this into a funky little "Lookup" class for two reasons:
- It's used in two places;
- I anticipate that we might need to add some sort of `explainWhy()` method if users find the heuristics confusing.
Test Plan: Created and updated revisions, saw them pick up the correct repository association. Ran Herald dry run against associable and nonassociable revisions, saw correct values populate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7147
Summary: This ends up living in HeraldAdapter even though its "task only" stuff. Reason being There are 4 or 5 functions that have little hooks; see diff. Ref T1638.
Test Plan: made a rule to assign tasks to me if made on web - great success. made a rule to assign tasks to other guy and add a project if title contained "foobar" - great success, including some confusion as ther two herald rules fought each other for task ownership.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1638
Differential Revision: https://secure.phabricator.com/D7146
Summary: ...and deploy on Maniphest. Ref T1638.
Test Plan: created a herald rule to be cc'd for tasks created via web. made a task via web and another via email and was cc'd appropriately. edited the herald to be cc'd for tasks created via not web. made 2 tasks again and got cc'd appropriately
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1638
Differential Revision: https://secure.phabricator.com/D7145
Summary: This isn't too useful most of the time since we don't automatically populate this data yet, but works fine.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7144
Summary: Ref T603. I think T2222 is fraught with peril so I'm not going to try to sequence it ahead of T603 for Differential. Provide access to policy controls in Differential's edit view.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7142
Summary: Adds some padding to the right
Test Plan: Looked at a diff
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7143
Summary:
Ref T2217. This partially retreads the ground from D7115.
- We're rendering silly transactions about descriptions when creating tasks. Hide those.
- Move the "created" transaction back to status. This fixes two things that are otherwise more of a mess than I'd anticipated:
- It fixes Reports without making a mess (see <https://github.com/facebook/phabricator/issues/395>).
- It renders old transactions properly (i.e., "created" instead of "reopened" for tasks older than the migration).
- Be explicit about action strength, so emails always say the most important thing in the subject.
Test Plan: Created and edited tasks, looked at resulting transactions, saw a cleaner transaction record.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7141
Summary: Ref T603. Makes the majority of reads policy aware (and pretty much all the important ones).
Test Plan:
- Created a comment with `differential.createcomment`.
- Created a new revision with `arc diff` in order to exercise `differential.creatediff`.
- Created an inline comment with `differential.createinline`.
- Added a comment to a revision.
- Edited an inline comment.
- Edited a revision.
- Wrote "Depends on ..." in a summary, saved, verified link was created.
- Browsed a file in Diffusion.
- Got past the code I changed in the Releeph request thing.
- Edited a Releeph request.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7136
Summary: Ref T603. Moves policy information from a custom field to the header for revisions.
Test Plan: Looked at a revision.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7135
Summary:
Ref T603. Read policies out of policy columns.
When a revision is associated with a repository (which is currently never), require view access on the repository to see the revision (or, require the viewer to be the owner). This is a blanket "do the right thing" rule which should make Differential's default policies align with user expectations.
Future diffs will populate the `repositoryPHID` when a revision is created.
Test Plan: Tooled around Differential. None of this stuff does anything yet, so nothing very exciting happened.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7134
Summary: Ref T603. Paves the way for policy controls.
Test Plan: Ran storage upgrade, bumbled around in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7133
Summary: Ref T603. Move to real Query classes.
Test Plan:
- Ran `phd debug pull X` (where `X` does not match a repository).
- Ran `phd debug pull Y` (where `Y` does match a repository).
- Ran `phd debug pull`.
- Ran `repository pull`.
- Ran `repository pull X`.
- Ran `repository pull Y`.
- Ran `repository discover`.
- Ran `repository delete`.
- Ran `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7137
Summary: Ref T603. This swaps almost all queries against the repository table over to be policy aware.
Test Plan:
- Made an audit comment on a commit.
- Ran `save_lint.php`.
- Looked up a commit with `diffusion.getcommits`.
- Looked up lint messages with `diffusion.getlintmessages`.
- Clicked an external/submodule in Diffusion.
- Viewed main lint and repository lint in Diffusion.
- Completed and validated Owners paths in Owners.
- Executed dry runs via Herald.
- Queried for package owners with `owners.query`.
- Viewed Owners package.
- Edited Owners package.
- Viewed Owners package list.
- Executed `repository.query`.
- Viewed "Repository" tool repository list.
- Edited Arcanist project.
- Hit "Delete" on repository (this just tells you to use the CLI).
- Created a repository.
- Edited a repository.
- Ran `bin/repository list`.
- Ran `bin/search index rGTESTff45d13dffcfb3ea85b03aac8cc36251cacdf01c`
- Pushed and parsed a commit.
- Skipped all the Drydock stuff, as it it's hard to test and isn't normally reachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7132
Summary:
We currently try to send Maniphest email "To" the owner and actor, but for unassigned tasks there is no owner.
Just filter the PHIDs in the parent, since it's reasonable for subclasses to be liberal about construction here.
Test Plan: Commented on an unassigned task, got an email without a bogus "To".
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7129
Summary:
Ref T603. Basically:
- Hide "Reports".
- Hide "batch edit" and "export to excel".
- Hide reprioritization controls.
- I left the edit controls, they show a "login to continue" dialog when hit.
- Allow tokenizer results to fill for public users.
- Fix a bug where membership in projects was computed incorrectly in certain cases.
- Add a unit test covering the project membership bug.
Test Plan: Viewed /maniphest/ when logged out, and while logged in.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7126
Summary:
Ref T603. Disable things the user can't use, allow logged-out users to get a reasonable version of the page.
Also allow logged-out users to view edit history of comments if they're able to see the object.
Test Plan: Viewed Maniphest detail as a logged-out user, got a largely sensible page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7124
Summary:
Ref T603. Adds policy controls to the task edit UI.
@chad, status + policy renders a little weird -- did I mess something up? See screenshot.
Test Plan: Edited policies, viewed a task.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7123
Summary:
Ref T603. Cleans up some obsolete stuff here:
- We no longer ever query by min/max priority (instead, `withPriorities(...)`).
- A parent class provides limit/offset.
- Result count is no longer reliable with policies. We could do "about X tasks" or something, but just drop it for now. There's only one remaining callsite anyway.
Test Plan:
- `grep`
- Viewed task list.
- Viewed a project page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7121
Summary:
Ref T603. Make almost every task read policy-aware. Notable exceptions are:
- Edge editor -- this stuff is prescreened and should be moved to ApplicationTransactions eventually anyway.
- Search/attach stuff -- this stuff needs some general work. The actual list should be fine since you can't pull handles. There may be a very indirect hole here where you could attach an object you can't see (but do know the ID of) to an object you can see. Pretty fluff.
- The "Tasks" field in Differential will let you reference objects you can't see. Possibly this is desirable, in the case of commandeering revisions. Mostly, it was inconvenient to get a viewer (I think).
Test Plan:
- Called `maniphest.info`.
- Called `maniphest.update`.
- Batch edited tasks.
- Dragged and dropped tasks to change subpriority.
- Subscribed and unsubscribed from a task.
- Edited a task.
- Created a task.
- Created a task with a parent.
- Created a task with a template.
- Previewed a task update.
- Commented on a task.
- Added a dependency.
- Searched for "T33" in object search dialog.
- Created a branch "T33", ran `arc diff`, verified link.
- Pushed a commit with "Fixes T33", verified close.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7119
Summary:
Ref T2217. Use `getLinkName()` instead of `getName()` so that we get, e.g.,
alincoln attached a revision: D123 Chop some logs
...instead of:
alincoln attached a revision: D123
Test Plan: Attached stuff, looked at the email, saw full object name.
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7127
Summary: adds padding to the maniphest status subheader
Test Plan: see padding in inspect element
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7125
Summary:
standing on the shoulders of the badass work to move Maniphest to ApplicationTransactions, this diff implements a few methods and adds an adapter class.
For now, we can add cc and flag tasks. I figure see what people ask for? Ref T1368.
Test Plan: created herald rules for title and description text hits. made tasks and verified CC and flags worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1368, T1638
Differential Revision: https://secure.phabricator.com/D7122
Summary: I'd like to reuse this for other content areas, renaming for now. This might be weird to keep setForm, but I can fix that later if we need.
Test Plan: reload a few forms in maniphest, projects, differential
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7120
Summary: Ref T2217. These checks are no longer necessary, ApplicationTransactions handle them for us.
Test Plan:
- Made a no-effect edit, verified no new transactions showed up.
- Made real edits, saw them happen and leave transactions.
- Made an edit which just reorders CCs, saw it detected as no-effect.
- As above, with projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7117
Summary:
Ref T2217. Fixes two issues:
# The "task created" email didn't include the task description, but should.
# We were treaging the "status" event as the "create", but that's kind of a mess. Treat the "title" event as the "create" instead. This makes initial emails say "[Created]".
Test Plan: Created some tasks, got better emails.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7115
Summary: These constants have moved to ManiphestTransaction. The other method only has one plausible callsite, just inline it.
Test Plan: Used Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7113
Summary:
There may be times when we don't want lines in some embeded source
code to be highlighted on click, this adds that functionality.
Also use the functionalty in `PasteEmbedView`
Test Plan:
- View paste, make sure everything works.
- Embed paste in comment, make sure everything works apart from click hl
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3881
Differential Revision: https://secure.phabricator.com/D7111
Summary: Name the token which was given in the feed story.
Test Plan: Gave/rescinded tokens. Looked at a feed story.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7110
Summary: Ref T2217. Render feed stories like "alincoln updated T123" instead of "alincoln updated this task.". Fix up some more translations.
Test Plan: Looked at feed, saw something a bit more reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7108
Summary: TimelineView on Maniphest is often kind of hard to parse because related/simultaneous transactions aren't visually grouped. Allow grouping. I'm going to clean this up a little bit more.
Test Plan: See screenshot.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7107
Summary: Ref T2217. Fixes T3865. Eventually we'll probably make more of this configurable, but for now shove custom fields down instead of sort of arbitrarily putting them in the middle.
Test Plan: Looked at a task with attached revisions and custom fields, saw custom fields last.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217, T3865
Differential Revision: https://secure.phabricator.com/D7103
Summary:
PHP_INT_MAX is rejected by ElasticSearch since it's outside of the representable integer range (see: <https://gist.github.com/JustinTulloss/c4ac0e1c93d6d1e91744>).
Just use 10K, as matching more than 10K results probably isn't useful to anyone.
Test Plan: Confirmed this fixes the issue in IRC. Ran a fulltext search.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7097
Summary: Ref T2217. Fixes T3877. Improves more behaviors in the presence of "no effect" detection and new comment storage.
Test Plan: Added CCs, added existing CCs, implicitly added CCs via reassign, added CCs with comment.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2217, T3877
Differential Revision: https://secure.phabricator.com/D7100
Summary: Ref T2217. Cleans up some of the "attached %d file(s)" stuff.
Test Plan: Generated some of these transactions and verified they render more naturally.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7096
Summary:
Ref T2217. Since we aren't actually using subscriptions yet, the "transactions have no effect" detection can trigger for `@mention`s of users who are already CC'd on a task.
Be more conservative about generating a CC transaction.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7095
Summary: Ref T2217. Cleans up the table names. Moves old data to `maniphest_transaction_legacy`. We'll drop that eventually once it's more clear that I didn't break the world.
Test Plan: Did reads/writes to/from these tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7094
Summary: Ref T2217. Pro is the new standard.
Test Plan: Lots of `grep`, made a pile of Maniphest views/edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7093
Summary: Ref T2217. The preview had the last callsite, nuke it.
Test Plan: Used preview. Grepped for `ManiphestTransaction(`, `ManiphestTransaction::`, `'ManiphestTransaction'`, `"ManiphestTransaction"`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7092
Summary:
`getArcanistProjectName()` has some logic which gets messy with the `self::ATTACHABLE` mechanism. This makes `differential.getdiff` and similar Conduit methods throw an exception when querying a diff which doesn't have a project. See <http://pastebin.com/Czzrd0Jz>.
Instead, unconditionally attach a project (possibly `null`) when loading diffs if they need projects.
Test Plan: Ran `differential.getdiff` against a `arc diff --raw` diff with no project, got a result instead of an exception.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, sttwister
Differential Revision: https://secure.phabricator.com/D7101
Summary: Adds status icons and colors to Maniphest and Differential. Also minor tweaks to them in hovercards. Probably some other stuff too.
Test Plan: Test many diff and task states.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7098
Summary:
Ref T2217. Removes most of the code from ManiphestTransactionEditor.
- Provides mail tag support in ManiphestTransactionEditorPro.
- There was one more write (subscribe/unsubscribe button) that I'd missed; modernize that.
Test Plan:
- Clicked subscribe/unsubscribe.
- Made some edits, verified mail had appropriate mail tags.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7091
Summary: Ref T2217. This is essentially the last writer, should be able to start deleting code now.
Test Plan: Used "Edit Task" to make a bunch of task edits.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7090
Summary:
"Branch" really means "repository main screen, with some branch selected", so a branch isn't actually required since we can just take you to the default.
Fixes an issue where new crumbs would throw an exception in SVN repositories.
Test Plan: Browed an SVN repo.
Reviewers: btrahan, mbishopim3
Reviewed By: mbishopim3
CC: aran
Differential Revision: https://secure.phabricator.com/D7099
Summary: When loading the cursor repository, we need to load the most recent
commit too if we're paging by commit date. This fixes a fatal for installs
with more than 100 repositories.
Auditors: btrahan
Summary: Ref T2217. When you add comments (or use that interface to make updates), ship it through the new code.
Test Plan: Added comments, made other changes to Maniphest tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7088
Summary: Ref T2217. Ship these through the new stuff.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7087
Summary: Ref T2217. Mail writes go through the new code now.
Test Plan: Shipped a bunch of mail in with `./bin/mail`, got reasonable edits as a result.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7086
Summary: Ref T2217. Nothing too surprising here. This transaction type is weird and should be replaced with the mainstream EDGE type at some point after things clear up more.
Test Plan: Attached and detached revisions and mocks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7085
Summary:
Ref T2217. Ship "Merge in Duplicates" through the new editor. The only notable thing here is `setContinueOnMissingFields()`.
The problem this solves is that if you add a custom field and mark it as required, all existing tasks are "invalid" since they don't have a value, and trying to edit them will raise an error like "Some Custom Field is required!". This is fine for normal edits via the UI, since the user can just select/provide a value, but surgical edits to specific fields should just ignore these errors. Add the ability to ignore these errors and use it on all the field-speific editors.
Test Plan: Merged duplicates, including "invalid" duplicates with missing fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7084
Summary:
Ref T2217. Swaps batch edits to modern editor.
Also, fix some issues with required fields and viewers being required to render certain standard fields (notably, date).
Test Plan: Made various batch edits, verified they went through properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7083
Summary:
Ref T2217. Drive reports out of the new table. Nothing too magical going on here.
Also fixes a bug with one of the links from reports.
Test Plan: Viewed reports.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7081
Summary: Ref T2217. All the reads route through new code already, start swapping writes over. This is the simplest writer, used when the user drag-and-drops stuff on the task list.
Test Plan: Dragged and dropped stuff across priorities. Got a transaction and some email. Verified the email and transaction looked OK, threaded properly, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7080
Summary: Ref T2217. No remaining callsites. Also get rid of some methods on ManiphestTransaction that nothing calls anymore.
Test Plan: `grep`, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7079
Summary: Ref T2217. Nuke this legacy callsite.
Test Plan: Loaded a task, looked at it. Looked the same as before.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7078
Summary: Ref T2217. These are mostly me making stuff up rather than some bird's-eye view of color and iconography across applications, yell if any of these seem off once this rolls out.
Test Plan:
- Looked at a bunch of transactions, saw reasonable looking colors and icons.
- Sent email, saw appropriate subject line actions.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7075
Summary: Ref T2217. Get rid of this rendering pathway's internals and move them to the modern stuff.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7072
Summary:
Ref T2217. This showed the text diff when you updated the description of a task, but is now obsolete.
Remove flags and methods related to rendering this pathway.
Test Plan: Clicked the fancy new "Show Details" instead.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7071
Summary: Ref T2217. Route transaction rendering through modern code. This just affects the detail page. Some rough edges but nothing significant.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7070
Summary:
Ref T2217. Move this off `LegacyQuery` and on to the real deal.
The rest of this patch mostly just replaces some gymnastics to get accurate-ish timestamps for CCs/Owners with `time()`. The search feature where edge time is stored was never really used and isn't necessarily of much value -- most indexers don't bother computing it exactly, and possibly we should get rid of it entirely. If it surfaces in the product again at some point, it's easy enough to make the time data more accurate and reindex.
Test Plan: Ran `bin/search index T12`, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7069
Summary:
Ref T2217. This is the risky, hard part; everything after this should be smooth sailing. This is //mostly// clean, except:
- The old format would opportunistically combine a comment with some other transaction type if it could. We no longer do that, so:
- When migrating, "edit" + "comment" transactions need to be split in two.
- When editing now, we should no longer combine these transaction types.
- These changes are pretty straightforward and low-impact.
- This migration promotes "auxiliary field" data to the new CustomField/StandardField format, so that's not a straight migration either. The formats are very similar, though.
Broadly, this takes the same attack that the auth migration did: proxy all the code through to the new storage. `ManiphestTransaction` is now just an API on top of `ManiphestTransactionPro`, which is the new storage format. The two formats are very similar, so this was mostly a straight copy from one table to the other.
Test Plan:
- Without performing the migration, made a bunch of edits and comments on tasks and verified the new code works correctly.
- Droped the test data and performed the migration.
- Looked at the resulting data for obvious discrepancies.
- Looked at a bunch of tasks and their transaction history.
- Used Conduit to pull transaction data.
- Edited task description and clicked "View Details" on transaction.
- Used batch editor.
- Made a bunch more edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7068
Summary:
Ref T2217. I'm going to do the fake-double-writes ("double reads"?) thing where we proxy the storage that worked pretty well for auth. That is:
- (Some more cleanup diffs next, maybe?)
- Move all the data to the new storage, and make `ManiphestTransaction` read and write by wrapping `ManiphestTransactionPro`.
- If nothing breaks, it's a straight shot to nuking ManiphestTransaction callsite by callsite.
I think Maniphest is way easier than Differential, because there are very few query sites and no inline comments.
Test Plan: `grep` to find callsites. Loaded task view, called Conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7067
Summary: Ref T2217. Add the tables and comment class for the new stuff. Not used yet.
Test Plan: Ran storage upgrade, browsed Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7066
Summary:
This is a mostly-faithful modernization of the Diffusion lint interfaces. It:
- Makes them policy aware;
- removes the last callsites for old/dead code (crumbs, nav).
It's a little rough, but should be perfectly usable. At some point this should get another pass, but probably after we make it easier to populate the lint data.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, FacebookPOC
Differential Revision: https://secure.phabricator.com/D7065
Summary: Fixes T903. Knock out the side nav, make it policy-aware, other minor cleanup.
Test Plan: See below.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T903
Differential Revision: https://secure.phabricator.com/D7064
Summary:
- Kicks it out to full width.
- More useful header/crumbs/properties/actions (needs some more work).
- Works for public repositories.
- Fix a bug where the "rX" crumb would lose the branch you're on.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7063
Summary: Get rid of remaining callsites for buildStandardPageResponse() and modernize the UIs.
Test Plan: Looked at branches, tags, and commit detail pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7062
Summary: Ref T603. Allows permitted users to set view and edit policies for repositories. So far the repository list, repository detail, repository edit, and browse interfaces respect these settings. Most other interfaces will respect stricter settings, but "Public" won't work. Lots of rough edges in the integration still. None of this makes policies any looser than they were already without explicit user intervention, so I just put a warning about it in the UI.
Test Plan: Set a repository to public and browsed it. Verified I could not access non-public repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, davidressman
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7061
Summary: Ref T603. Make common repository queries (in Conduit and DiffusionRequest) policy-aware. These tend to get caugh by something else anyway, but tighten them up.
Test Plan: The conduit change already provided `user` everywhere. I verified that and browsed some pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7060
Summary:
Ref T603. See inlines for an explanation. The case where I hit this was loading the "Pending Differential Revisions" panel in Diffusion when logged out, after making a repository public.
What happens is that we load 10 revisions (say, D1 .. D10) but the user can't see any of them. We then try to load the next 10, but since the pagination is ordered by date modified, we need to base the next query on the modified date of the last thing we loaded (D10). However, since we use the viewer's policies to load that cursor object, it fails to load, and then we just issue the same query over and over again, loading D1 .. D10 until we run out of execution time.
Test Plan: Interface now loads correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7059
Summary: Improves transaction rendering for custom fields and standard custom fields.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7054
Summary:
- Add some TODO'd keys.
- Add policy fields.
Test Plan: Viewed repositories; created a new repository and verified it got the right default policy settings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7056
Summary: See task
Test Plan:
Attempt to signup with recaptcha disabled.
Attempt to signup with recaptcha enabled with incorrect value.
Attempt to signup with recaptcha enabled with correct value.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3832
Differential Revision: https://secure.phabricator.com/D7053
Summary: We currently render something kind of goofy; integrate these with the other actions.
Test Plan: Viewed `aphlict.swf`, some PNG in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7052
Summary:
We have this silly "view" preference which has a variety of silly values: "plain", "plainblame", "highlighted", and "blame", and then also "raw", which is magical. This is really just two flags: color on/off, and blame on/off (plus a separate mode for raw).
Express the code in terms of the flags and, e.g., get rid of the state transition tables we had before.
Test Plan: Viewed code in all four modes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7046
Summary: This needs some more cleanup, but gets us a step closer to something reasonable.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7043
Summary: Broadly, I'm trying to modernize these views and fix UI and at least mitigate mobile problems. See discussion.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7042
Summary: Get thee modernized.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7040
Summary: Instead of rendering this in all callers, just pass the object into the header and let it figure out how to format it.
Test Plan: Looked at Legalpad, Paste, and Pholio.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7039
Summary: Allows the user to query for repos by VCS type.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7038
Summary: Gets rid of as much of this as possible. We'll batch handles and remarkup again some day, but after ApplicationTransactions.
Test Plan: Edited, viewed, and checked email for custom field edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7037
Summary: Ref T3794. Drop auxiliary field, use standard field.
Test Plan: Performed migration, field seemed to survive it intact. Edited and viewed tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3794
Differential Revision: https://secure.phabricator.com/D7036
Summary: Makes Maniphest look more like standard fields to make the migration easier.
Test Plan: Edited tasks and users with required and invalid fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7035
Summary: Make Maniphest use the standard API, `renderEditControl`. Removes custom method `renderControl`.
Test Plan: Created/edited tasks with custom fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7034
Summary: Adds policy headers to more (all?) places currently in use.
Test Plan: test each page changed.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7032
Summary:
Ref T418. This is fairly messy, but basically:
- Add a validation phase to TransactionEditor.
- Add a validation phase to CustomField.
- Bring it to StandardField.
- Add validation logic for the int field.
- Provide support in related classes.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D7028
Summary:
If handed a revision ID, we might get more than one result, which causes `executeOne()` to throw. Instead, translate the revision id into a diff ID before querying for the diff.
Also one small consistency change to parameter casing.
Test Plan: Used console to query for a revision with more than one diff using the revision id.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Differential Revision: https://secure.phabricator.com/D7026
Summary: Also some random cleanup now and again. Note reply handler stuff is kind of bojangles bad right now. It didn't work before though either so hey.
Test Plan: asked questions, answered questions, edited answers... the feed pleased my eye
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3653
Differential Revision: https://secure.phabricator.com/D7027
Summary: We were returning an array here when previous return was a string.
Test Plan: reload diff
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7025
Summary: Fixes T3840. Depends on D7021. See task for discussion. Also improved some config/help stuff.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3840
Differential Revision: https://secure.phabricator.com/D7022
Summary:
Conduit has a query to make a draft inline comment, but createcomment doesn't have the ability to attach them.
Added optional parameter to attach any existing draft comments. Default value is false, so existing api users won't be effected by the change.
Test Plan: Tested no draft comments and multiple draft comments, attach_inlines =true, false, and empty.
Reviewers: vrana
Reviewed By: vrana
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D7019
Summary:
- D6966 accidentally reversed the order of `$diffs`. Reverse it back.
- The new policy header stuff returns `array(icon, text)` but gets `strlen()`'d by a caller. Silence that warning for now.
Test Plan: Created a revision with several diffs. Saw them in the right order; saw no warning on the diff attach screen.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran, mbishopim3
Differential Revision: https://secure.phabricator.com/D7023
Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this. Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823.
Test Plan: used the new api via test console - great success.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3823
Differential Revision: https://secure.phabricator.com/D6966
Summary: Ref T3583. Fixes T3835. Dropbox and Disqus both want these things back, so restore them until we can do something about T3583.
Test Plan: Viewed homepage, clicked "View All X" buttons.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3583, T3835
Differential Revision: https://secure.phabricator.com/D7017
Summary: This works fine for custom queries, but not for builtins.
Test Plan: Exported a builtin query.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7015
Summary:
A couple of things here:
- These links got fixed, but they show all user or project tasks. They should show only open ones.
- Add an anchor so we jump you straight to the results, since the query UI is like a thousand miles tall now. We might take some other approaches here too, but let's see if this feels reasonable.
Test Plan: Clicked "View Tasks" from Profile and Projects. Executed some queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: euresti, aran, chad
Differential Revision: https://secure.phabricator.com/D7014
Summary: The adds the ability to set 'properties' such as state, privacy, due date to the header of objects.
Test Plan: Implemented in Paste, Pholio. Tested various states.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7016
Summary: There's a bunch of stuff that lives only in AuxiliaryField which is called on objects which may be ManiphestCustomFields right now. This is basically a list of remaining API methods which need to be moved to the new stuff. This enables construction of new-style custom fields.
Test Plan: Created a sophisticated Maniphest custom field.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7013
Summary: These end up a little weird with subclassing instead of `switch`, but some day we could alias them to one another or something I guess. If I'm feeling brave, I might get rid of the "user" variant when I migrate Maniphest custom field specs, and turn it into "users, limit = 1" or something like that.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7010
Summary: See previous revisions. As maniphest.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7009
Summary: Ref T418. Although Maniphest does not use ApplicationTransactions, we can fake a lot of it and provide a more uniform API. Deletes as much custom code from Maniphest as possible along the edit workflows, using core code instead.
Test Plan:
With custom fields:
- Edited a task.
- Created a task.
- Queried a task with Maniphest.
- Updated a task with Maniphest.
- Used `?template=nnn` to create a similar task.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D7001
Summary: Ref T418. Run all the meaningful stuff on the detail page out of shared code.
Test Plan: Looked at detail page, saw custom fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D7000
Summary: Ref T418. Moves data from the Maniphest-specific table to the general one. This patch is a bit gross, but mostly about getting the reads and writes aimed correctly. Future patches will clean things up.
Test Plan: Migrated data across formats. Verified it survied the migration. Viewed and edited tasks' custom fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D6999
Summary: Ref T418. Maniphest has an obsolete class-based field selector. Replace it with CustomField-based selectors, which use the nice config UI and are generally way easier to use.
Test Plan: Added custom fields; edited and viewed custom fields on tasks. Everything worked as expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D6998
Summary: Ref T418. Depends on D6992. This adds index and value storage for Maniphest custom fields.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D6995
Summary:
Ref T2625. Ref T3794. Ref T418. Ref T1703.
This is a more general version of D5278. It expands CustomField support to include real integration with ApplicationSearch.
Broadly, custom fields may elect to:
- build indicies when objects are updated;
- populate ApplicationSearch forms with new controls;
- read inputs entered into those controls out of the request; and
- apply constraints to search queries.
Some utility/helper stuff is provided to make this easier. This part could be cleaner, but seems reasonable for a first cut. In particular, the Query and SearchEngine must manually call all the hooks right now instead of everything happening magically. I think that's fine for the moment; they're pretty easy to get right.
Test Plan:
I added a new searchable "Company" field to People:
{F58229}
This also cleaned up the disable/reorder view a little bit:
{F58230}
As it did before, this field appears on the edit screen:
{F58231}
However, because it has `search`, it also appears on the search screen:
{F58232}
When queried, it returns the expected results:
{F58233}
And the actually good bit of all this is that the query can take advantage of indexes:
mysql> explain SELECT * FROM `user` user JOIN `user_customfieldstringindex` `appsearch_0` ON `appsearch_0`.objectPHID = user.phid AND `appsearch_0`.indexKey = 'mk3Ndy476ge6' AND `appsearch_0`.indexValue IN ('phacility') ORDER BY user.id DESC LIMIT 101;
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| 1 | SIMPLE | appsearch_0 | ref | key_join,key_find | key_find | 232 | const,const | 1 | Using where; Using temporary; Using filesort |
| 1 | SIMPLE | user | eq_ref | phid | phid | 194 | phabricator2_user.appsearch_0.objectPHID | 1 | |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
2 rows in set (0.00 sec)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T1703, T2625, T3794
Differential Revision: https://secure.phabricator.com/D6992
Summary:
Currently, these events don't fire for Conduit updates, which makes them sort of silly.
This will get proper treatment after T2222.
Test Plan: Installed a `throw new Exception(...)` event listener. Performed Conduit and web updates of revisions, saw event listener fire.
Reviewers: btrahan, guywarner
Reviewed By: guywarner
CC: aran
Differential Revision: https://secure.phabricator.com/D7004
Summary: Fixes T3833. Serious business was seriously disrupted.
Test Plan: Looked at button in both seriousness modes.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3833
Differential Revision: https://secure.phabricator.com/D7003
Summary: This class is no longer used. It has no callsites.
Test Plan: `grep`
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6996
Summary: A few things link to old URIs for Maniphest, update them.
Test Plan: Clicked all the things.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6989
Summary: Deploy on paste and macro for create stories, 'cuz those are boring emails. Fixes T3808.
Test Plan: made a paste and a macro. commented on 'em. verified i got mail on comments only.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3808
Differential Revision: https://secure.phabricator.com/D6988
Summary: Pagers in Maniphest (and, to some degree, apps like Pholio) get lost a bit. Put them in a little box.
Test Plan: Looked at Maniphest and Pholio, pager was more obvious and less un-designed-looking.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6987
Summary: reported by csilvers in irc
Test Plan: ran a bum query with --trace and verified table scan not run
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6986
Summary:
Removes a bunch of dead stuff:
- Old side nav with hard-coded filters.
- Old edit/list/delete/update interfaces for those filters.
- Old `buildStandardPageResponse()`.
- Some other junk with no callsites.
- Reduce the number of places where the "Create Task" button is built.
Test Plan: `grep`; used list view, batch editor, reports.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6985
Summary: Drive these purely out of configuration after removing behavioral hardcodes in D6981.
Test Plan:
Mucked around with them:
{F58128} {F58129} {F58130}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D6984
Summary: Accidentally lost this in the melee. Put it back.
Test Plan: Saw link, then clicked it. Great success!
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6982
Summary:
Ref T3583. Currently, we have some hard-coded behaviors associated with the "Unbreak Now" and "Needs Triage" priorities. Remove them:
- Users seem somewhat confused by these on occasion, and never seem to think they're cool/useful (that I've seen, at least).
- I think they have low utility in general, see T3583.
- Saves three queries on the home page, which can no longer use row counting since they must be policy filtered.
- Primarily, this paves the way for allowing installs to customize priorities, which is an occasional request.
Also deletes a lot of code with no callsites.
Test Plan: Mostly `grep`. Loaded home page. Viewed reports and task list.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D6981
Summary: This marks the first time in history that "Pro" has been removed.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6980
Summary: Point these at the new data and URIs.
Test Plan:
- Batch edited some tasks.
- Exported some tasks to excel.
{F58112}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6978
Summary:
Current page size is `1000`. This is nice to have in some cases, but makes pages slower than necessary in others. Task lists are generally dominated by rendering costs.
For example, my default is "recent tasks", which just lists all tasks ordered by date created. Showing 100 tasks here instead of 1000 makes this several times faster without compromising utility.
I don't want to force the default to 100, though, since sometimes listing everything is quite useful and I think an advantage of Maniphest is that it generally deals reasonably well with large task sets.
(This `limit` property is actually read by the default implementation of `getPageSize()` in the parent class.)
Test Plan: Made queries with page sizes 1, 100, 12, 9, 3000, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6976
Summary: Ref T2625. Fixes user and project paging. Adds visibility-aware project group filtering.
Test Plan: Set page size very small and paged forward and backward in Maniphest, particularly with "Assigned" and "Project" group-by filters.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6973
Summary:
Ref T2625. Depends on D6971. Maniphest is complicated to implement cursor paging for. Builds on D6971 to do so.
This is //almost// complete. Paging on projects and authors doesn't quite work, I'll clean that up shortly. Left some TODOs.
Test Plan: Set page size to `3`, paged forward and backward in a bunch of group/order modes. Results seemed to be as expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6972
Summary:
We currently have two giant messes for paging across multiple columns (usually because one column is not unique), and I'm about to add a third for Maniphest.
Provide a more structured way to build these `A > a OR (A = a AND B > b)` clauses.
Test Plan: Set page size to `2` for Differential and Diffusion and paged forward and backward with a bunch of different orders set. Pages worked as expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6971
Summary: This allows administrative overreach. Administrators can enable `javascript:` and then XSS things if this isn't locked.
Test Plan: Viewed value on web UI, verified it was locked.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6975
Summary: Fixes T3825. See that task for details.
Test Plan: Verified that `#\herp` no longer matches project `#herp`, but `#herp` still works fine.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3825
Differential Revision: https://secure.phabricator.com/D6970
Summary:
Fixes T3807. Several issues:
- Currently, we split config of type `list<string>` on commas, which makes it impossible to enter a regex with a comma in it.
- Split on newlines only.
- Some of the examples are confusing (provided in JSON instead of the format you actually have to enter them).
- Show examples in the same format you should enter text.
- We didn't validate regexps.
- Introduce `list<regex>` to validate regexes.
@hlau: Note that the old config format for the bugtraq stuff implied the delimiters on the regular expression. They are no longer implied. The examples show the correct format.
Test Plan: Viewed and edited affected config, hitting error and success cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: hlau, aran
Maniphest Tasks: T3807
Differential Revision: https://secure.phabricator.com/D6969
Summary:
Fixes T3821. Maybe. The existing code seemed to have a bug and actually return the //commit phid//. Judging by the function name this is not intended.
Also, sorry to step on toes here -- I thought no one was assigned and was curious about loadRelativeEdges and here we are...
Test Plan: lots of logic here as I have no idea how to use Releeph.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3821
Differential Revision: https://secure.phabricator.com/D6967
Summary: Adds the small caret to differential. Cleans up dropdown frame.
Test Plan: Test caret in differential.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6983
Summary: Swaps the rendering over to the current rendering. This is mostly copy/paste out of TaskListController, which is going to get nuked, with some cleanup.
Test Plan:
{F58064}
- Ran a bunch of queries.
- Viewed empty states.
- Drag-and-dropped stuff.
- (Batch editor / excel export need a tweak to run the new-style queries.)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6961
Summary: Fixes T1485.
Test Plan: made a herald rule for "not exists". committed to master with no diff. audit was triggered
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1485
Differential Revision: https://secure.phabricator.com/D6964
Summary: Followup to D6924. Fixes T3824.
Test Plan: deleted a file in a diff. was able to view file content without JS errors
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3824
Differential Revision: https://secure.phabricator.com/D6963
Summary: This is the last missing filter.
Test Plan: Grouped results by a bunch of stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6960
Summary: I think the old thing did this, but this makes queries a bit less ridiculous. For example, `secure.phabricator.com` currently issues a query for 664 handles on my task list, but only 73 of them are unique (basically, all the projects plus all the authors). This proably is slightly good for performance, but mostly makes the "Services" tab manageable.
Test Plan: Looked at Maniphest and some other pages, saw handles and objects where they were expected to be.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6959
Summary:
See discussion in D6955. Currently, the logic for "Group by: Project" is roughly:
- Load every possible result.
- Lots of in-process garbage.
Instead, use the new local project name index (from D6957) to service this query more reasonably. Basically:
- Join a table which has keyed project names.
- Order by that table.
Test Plan: {F58033}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6958
Summary: See discussion in D6955. This provides a table we can JOIN against to (effectively) "ORDER BY project name", populates it intially, and keeps it up to date as projects are edited.
Test Plan:
- Ran storage upgrade, verified projects populated into the table.
- Edited a project, verified its entry updated.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6957
Summary:
See discussion in D6955. Provide an event for applications and users to update secondary search indexes.
Facebook: I don't recall exactly how all the search stuff is rigged up, but this might provide a more practical / less fragile alternative. I think it publishes into ElasticSearch now, and then intern somehow handles the result merge at display time, implictly relying on Phabricator's storage format? A cleaner approach might be to publish a secondary "intern" index in a standard format.
Test Plan: Ran `bin/search index --type proj --trace`, saw events fire.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D6956
Summary:
Part one of a large and complicated plot:
- The last filter for Maniphest "pro" queries is "Group By".
- This is currently executed in a convoluted and ridiculous way, loading massive amounts of data.
- The primary reason it works like it does is that we don't have a project name index available in Maniphest, so we can't sort in the DB.
- So, I want to provide a name index to Maniphest and push this work to the DB.
To do that, my plan is:
- Index projects in Search.
- Add a "did update index" event.
- Have Maniphest listen for it.
- When projects are updated, update their indexes in Maniphest.
- Rewrite the giant mess of "group by: project" to be somewhat reasonable.
- This may also extend to some future "group by: assignee".
This is the first small step down this path, which just indexes projects in search.
Test Plan: Ran `bin/search index --type project`, then searched for projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6955
Summary: Depends on D6952. Unpunts there since I'm rolling into a swamp full of schema changes.
Test Plan: Issued date-constrained query and saw key as a candidate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6954
Summary: Noticed this in the schema. "Touches" were an idea that never really got off the ground, as we built out more/better notification channels instead. Essentially, they recorded any object you'd ever interacted with. Maybe this will be useful some day, but for now it does nothing and can't be interacted with. Nuke it.
Test Plan: `grep`, loaded Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6953
Summary: Adds date created filtering. There's a task for this somewhere that I can't immediately find.
Test Plan: Filtered tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6952
Summary: Restores any/all/user/exclude project filters to the new search.
Test Plan: Filtered stuff by projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6951
Summary: Restores this field to the new ApplicationSearch-based search.
Test Plan: Used fulltext search to find tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6950
Summary: Move this into a more consistent location.
Test Plan: Loaded Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6949
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))
Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, FacebookPOC
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6941
Summary: Ref T603. Ref D6941.
Test Plan: Clicked around all over - looked good. I plan to re-test D6941 to make sure the executeOne case works now as intended
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6944
Summary: Ref T2625. Further expands the "pro" search.
Test Plan: Used new options to query tasks.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6935
Summary: Ref T2625. Moves this a step toward being able to replace the current search.
Test Plan: Used search interface.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6934
Summary:
Ref T603. Ref T2625. Cutting this over is tricky because of Maniphest's existing saved queries. Plan here is:
- Build out the "pro" controller at `/maniphest/query/`.
- Once it's at parity, migrate custom queries.
- Nuke the old UI.
This provides a minimal implementation with no filter support.
Test Plan: Looked at `/maniphest/query/`, saw results technically available.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6933
Summary:
Ref T603. Moves to detangle and optimize how we apply policies to filtering objects. Notably:
- Add a short circuit for omnipotent users.
- When performing project filtering, do a stricter check for user membership. We don't actually care if the user can see the project or not according to other policy constraints, and checking if they can may be complicated.
- When performing project filtering, do a local check to see if we're filtering the project itself. This is a common case (a project editable by members of itself, for example) and we can skip queries when it is satisfied.
- Don't perform policy filtering in ObjectQuery. All the data it aggregates is already filtered correctly.
- Clean up a little bit of stuff in Feed.
Test Plan: Pages like the Maniphest task list and Project profile pages now issue dramatically fewer queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6931
Summary: Ref T603. These were deprecated some time ago in favor of the more standard withIDs() / withPHIDs().
Test Plan: `grep`, loaded some interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6929
Summary: Ref T603. Prepare for conversion to a policy-aware query.
Test Plan: Browsed various interfaces which use this stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6928
Summary:
Ref T418. These implementations share no method names, so we can safely just move Maniphest fields into the `PhabricatorCustomField` hierarchy.
Replaces two Maniphest-specific custom field exceptions which nothing catches.
Test Plan: Viewed Maniphest, edited/altered custom fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D6927
Summary: Simplify rendering of the repository list. For inactive repositories, mark them disabled.
Test Plan: {F57615}
Reviewers: btrahan, rockybean
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6921
Summary:
These need to die soon since they're not structurally policy-aware, but keep them around for the moment until we can replace them.
There is no UI to create these, and only Facebook has them.
Test Plan: {F57614}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6920
Summary: Fixes T2298. Allows repositories to be ordered by name, callsign, commit, or date created. Slightly messy because of cursor paging.
Test Plan: Sorted commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2298
Differential Revision: https://secure.phabricator.com/D6919
Summary: Adds a status filter and makes the default query "active" repositories.
Test Plan: Used new filter to execute queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6918
Summary:
Ref T2625. Switches Diffusion to ApplicationSearch. Notes:
- Rendering is a bit rough, I'll clean that up next.
- Ordering is a bit arbitrary, also coming shortly.
Test Plan: Used `/diffusion/` to execute various searches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6917
Summary: We should bring these back some day, but they should be denormalized, inside the query, and there should be a better pipeline to build them in the first place. Just get rid of them for now; this essentially impacts only us.
Test Plan: Loaded `/diffusion/`, same page minus lint counts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, vrana
Differential Revision: https://secure.phabricator.com/D6915
Summary: Ref T2625. `DiffusionHomeController` currently runs these queries inline. Move them into `DiffusionRepositoryQuery`. Prepareds for ApplicationSearch.
Test Plan: Loaded `/diffusion/`, saw the same content as before.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6914
Summary:
Ref T3687. JIRA is able to piggyback on a fair amount of Asana infrastructure, but the voicing we use on Asana tasks (which are always about one object) isn't very good for JIRA issues (which may have many linked objects). Specifically, we publish stories like this to Asana:
alincoln accepted this revision.
This is meaningless in JIRA since you have no idea what it's talking about. Instead, publish like this:
alincoln accepted D999: Put a bird on it
Additionally, supplement it with a URI, so the total story text we publish is:
alincoln accepted D999: Put a bird on it
https://phabricator.whitehouse.gov/D999
Signifcantly less useless!
Test Plan: {F57523} {F57524}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6907
Summary:
Ref T3687. See some discussion in D6892. The JIRA doorkeeper publisher shares a reasonable amount of code with the Asana publisher. Remedy this:
- Create `DoorkeeperFeedWorker`, where shared functionality lives (mostly related to building story context objects).
- Push responsibility for enabling/disabling a worker into this new layer, via `isEnabled()`. This allows `FeedPublisherWorker` to dynamically find and schedule doorkeeper publishers, so third parties can add additional doorkeeper publishers.
- Some general cleanup/documentation.
Test Plan: Used `bin/feed republish` to republish stories about objects with JIRA and Asana links. Verified that doorkeeper publishers activated properly, made calls, and published events into the remote systems.
Reviewers: btrahan, akopanev22
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6906
Summary: the attachX upgrade means we need to blank this out formally when creating a new object. Fixes https://github.com/facebook/phabricator/issues/383
Test Plan: loaded phortune for the first time - no fatal and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6939
Summary: Slightly more readable, less space than current index. LMK if you hate it though.
Test Plan: Look at user and dev book indexes.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6932
Summary: Moves book view to use PHUIDocument, fix some other spacing issues.
Test Plan: Review a number of pages in Diviner.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6925
Summary:
Adds most of Diffusion's commenting options available in the web UI
Mark method as deprecated immediately per @epriestley's request
Test Plan:
Used the Conduit web console to check:
* Lookup by PHID works
* Error is raised if commit by PHID is not found
* "action" validation works and raises appropriate error
* "message" raises error if empty
* Actions to raise concern or accept commit work
* Method is marked as deprecated from the start
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6923
Summary: Adds plain support for object lists that just look like lists
Test Plan: review UIexamples and a number of other applications
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6922
Summary:
Fixes T3810. In PhabricatorPeopleQuery, we issue an unnecessary query like this:
SELECT f.* FROM file f WHERE (f.phid IN ('')) ORDER BY f.id DESC
...if we're loading a user without a profile picture. Filter the file PHIDs before loading them to prevent this.
This doesn't change anything, but saves us a spurious/silly query.
Also makes `PhabricatorPeopleProfileController` use `needProfileImage()`, moving us closer to getting rid of `loadProfileImageURI()` eventually.
Test Plan: Looked at profiles of users with and without profile pictures. Checked query log in DarkConsole.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3810
Differential Revision: https://secure.phabricator.com/D6913
Summary:
Ref T988.
- Render "Implements:" as tags, too.
- Minor CSS tweak to tags in property lists.
- Add a bunch of group patterns to the Phabricator book.
- Fix some stuff with how hashes are computed and cached.
- Minor tweak to reuse the Diviner engine for slightly improved performance.
Test Plan: Regenerated and looked at documentation.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3811, T988
Differential Revision: https://secure.phabricator.com/D6912
Summary:
Ref T988. Not sure about this, feel free to push back or tweak it or whatever, but I want to reduce the amount of meta-text in the method documentation. Primarily this:
- Shortens "From parent implementation in ClassName:" to "ClassName".
- Tries to tweak the styles a bit so that it's relatively obvious what that means (hopefully?).
- Fixes an issue with tasks where some methods could be ignored.
Test Plan: {F57565}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6911
Summary: Ref T988. Show "Extends:" as linked tags. Fix the style of "This <top-level thing, like a class or function>" is not documented so it's the same as "This method is not documented.".
Test Plan:
Tags thing before:
{F57557}
Tags thing after:
{F57558}
Undoc before:
{F57559}
Undoc after:
{F57560}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6910
Summary: Ref T988. Make this more useful, and link it to the methods it describes.
Test Plan:
Before:
{F57553}
After:
{F57554}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6909
Summary:
Ref T988. Instead of rendering this:
ClassName
final class ClassName
methodName
final public function methodName(...)
...just render this:
final class ClassName
final public function methodName(...)
Also link and anchor the method names.
Test Plan:
Before:
{F57536}
{F57537}
After:
{F57538}
{F57539}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6908
Summary: This is just renaming to PHUI (I like shorter text :)
Test Plan: reload workboard examples page, seems to not fatal and looks very appealing
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6904
Summary: See IRC. We already have "after", add the corresponding "before". This makes polling for updates much easier.
Test Plan: Ran queries with "before" and "after".
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6903
Summary: Fixes T3447 - adds check if slug is projects/ alerts user
Test Plan: Create new doc in Phriction. Type projects/whatever and get error
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3447
Differential Revision: https://secure.phabricator.com/D6901
Summary: See IRC. We currently render a "show all changes" button for commits which have more than 100 but fewer than 1000 changes, but it doesn't actually do anything. Make it do what it's supposed to.
Test Plan: Set the limit to 2; clicked the button.
Reviewers: chad, staticshock, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6900
Summary:
Ref T3687. Publish stories into JIRA.
These need some voicing fixes, which maybe involves straightening out the feed code. For example, they're voiced in-context ("updated this revision") when they should be voiced out-of-context ("updated D123").
Generally, this is similar to the Asana stuff but a lot simpler since we don't need to do any state management.
Test Plan: {F57366}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6892
Summary: Ref T3687. The `value` property may be `null`.
Test Plan: Loaded a revision with the JIRA field enabled but no issues attached, no longer saw a warning about a bad argument to `foreach()`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6890
Summary: this ends up being a little weird since you can't actually edit files. Also, since we create files all sorts of ways, sometimes without even having a user, we don't bother logging transactions for those events. Fixes T3651. Turns out this work is important for T3612, which is a priority of mine to help get Pholio out the door.
Test Plan: left a comment on a file. it worked! use bin/mail to verify mail content looked correct.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, wez
Maniphest Tasks: T3651, T3612
Differential Revision: https://secure.phabricator.com/D6789
Summary: This adds a number of new styles for Diviner documentation. Not sure I've covered all the bases or wrote this in the most efficient manner, but passing it along now for early review before tightening everything up.
Test Plan: Review various class pages.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6888
Summary: Fixes T3798. Macros now show up as manually uploaded
Test Plan: Upload a macro, go to file, new macro is visiable
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3798
Differential Revision: https://secure.phabricator.com/D6887
Summary: Fixes T3792. These raise errors if the database is in strict mode and you try to create an "any" rule.
Test Plan: Created a rule with "any".
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3792
Differential Revision: https://secure.phabricator.com/D6883
Summary: I left a clamp in the patient.
Test Plan: derp-a-derpderp
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6882
Summary:
Previously, maniphest tasks would get upated by diffs on branches
with tasky names, even if maniphest was disabled.
Test Plan:
Tested createing a diff in sandbox with maniphest disabled, on a
git branch named using the format "t###". Without this change,
if there happened to be a task in the maniphest DB which matched,
it was updated an email was sent to users.
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, slawekbiel, whhone, Korvin, aran
Differential Revision: https://secure.phabricator.com/D6881
Summary:
Ref T3687. This adds a field which allows you to link Differential Revisions to JIRA issues.
This is just about as basic as it can get, but gets the job done. The field enables itself if you have a JIRA auth provide. You enter JIRA issues in a comma-delimited format and it generates appropriate edges.
Nothing is pushed to the issues yet.
The only real rough part here is that if you commandeer a revision which is linked to issues you can't see, editing it is difficult via the CLI. This seems pretty much like a non-issue, but at some point we can let the field throw some kind of "RecoverableInvalidFieldException" which just warns the user. The "no reviewers, continue anyway?" prompt could then use that too.
Test Plan:
- Edited via web UI, tried valid/invalid edits, checked that edges showed up in the database, added/removed issues, clicked issue links.
- Edited via CLI, tried valid/invalid edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6879
Summary:
Ref T3687. Adds a Doorkeeper bridge for JIRA issues, plus remarkup support. In particular:
- The Asana and JIRA remarkup rules shared most of their implementation, so I refactored what I could into a base class.
- Actual bridge implementation is straightforward and similar to Asana, although probably not similar enough to really justify refactoring.
Test Plan:
- When logged in as a JIRA-connected user, pasted a JIRA issue link and saw it enriched at rendering time.
- Logged in and out with JIRA.
- Tested an Asana link, too (seems I haven't broken anything).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6878
Summary: Ref T3687. These buttons don't work quite the same way, but are similar enough that the code seems worth consolidating.
Test Plan: Viewed and clicked both OAuth1 (Twitter, JIRA) and OAuth2 (Facebook) login buttons. Got logins.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6874
Summary: Depends on D6872. Ref T3687. Give the user a nice dialog instead of a bare exception.
Test Plan: Cancelled out of Twitter and JIRA workflows. We should probably do this for the OAuth2 workflows too, but they're a bit of a pain to de-auth and I am lazy.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6873
Summary: Starting to roll out the standard colors and spacing to action list, headers, and property views. Also softened the grey borders a hex.
Test Plan: Review Maniphest and Differential on desktop and mobile. Felt the flow of standardization waft over me.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6869
Summary:
Ref T3599
Go through everything, grep a bit, replace some bits.
Test Plan: Navigate around a bit
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3599
Differential Revision: https://secure.phabricator.com/D6871
Summary:
Ref T3687. Depends on D6867. This allows login/registration through JIRA.
The notable difference between this and other providers is that we need to do configuration in two stages, since we need to generate and save a public/private keypair before we can give the user configuration instructions, which takes several seconds and can't change once we've told them to do it.
To this effect, the edit form renders two separate stages, a "setup" stage and a "configure" stage. In the setup stage the user identifies the install and provides the URL. They hit save, we generate a keypair, and take them to the configure stage. In the configure stage, they're walked through setting up all the keys. This ends up feeling a touch rough, but overall pretty reasonable, and we haven't lost much generality.
Test Plan: {F57059}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6868
Summary: Ref T3687. Depends on D6864. Implements the `OAuth1` provider in Phabricator (which is mostly similar to the OAuth2 provider, but doesn't share quite enough code to actually extend a common base class, I think) and Twitter as a concrete subclass.
Test Plan:
Created a Twitter provider. Registered, logged in, linked, refreshed account link.
{F57054}
{F57056}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6865
Summary: More grey tweaks, breaking these up so I can test and tweak each as needed.
Test Plan: Review pages.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6866
Summary: Also, don't try to load prefs for non-users.
Test Plan: toggle, save, look at something with a time. arc unit.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6796
Summary:
Ref T988. Currently, every class/function needs to be annotated with `@group`, but 99% of this data can be inferred from file structure, at least in this project. Allow group specifications like:
"paste" : {
"name" : "Paste",
"include" : "(^src/applications/paste/)"
}
..to automatically put everything defined there in the "paste" group. A list of regexps is also supported. Depends on D6855.
Test Plan: Regenerated documentation with `bin/diviner generate --book src/docs/book/phabricator.book --clean`, observed all Paste stuff go in the paste group.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6856
Summary: This adds standard 'blues' and start integration of standard colors for text, backgrounds, and borders.
Test Plan: sb
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6857
Summary: Fixes T3786. Not 100% sold on this (I don't want to restore all of the original filters, since users can and should just build the weird ones if they use them), but this is almost certainly the most useful of the defaults which ApplicationSearch removed.
Test Plan: Viewed `/differential/`, executed the query.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3786
Differential Revision: https://secure.phabricator.com/D6860
Summary: This adds a set of standard grey colors for use in shading objects and importance. I'll follow up and start implementing in another diff.
Test Plan: Color UI Examples, Adobe Kuler
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6853
Summary: Ref T988. Adds support for the "abstract" and "final" keywords in the atomizer.
Test Plan: Looked at abstract/final stuff.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6850
Summary:
Ref T988. As mentioned elsewhere, one broad goal of this iteration is to reduce the amount of boilerplate documentation we need to write. For example:
- I want to set `@group` by default in most cases.
- I want to inherit things like `@param` and `@return` by default.
- I want to inherit method documentation by default.
- I want to inherit `@task` information.
This implements most of the method inheritance stuff.
This //looks// super gross, but I believe we now compose all of the information of interest at display time and can work on rendering it sensibly in the near future.
Test Plan: {F56790}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, sascha-egerer
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6849
Summary: Currently, adapters can only fail mail temporarily. Allow them to indicate a permanent failure by throwing a special exception.
Test Plan: Added and ran unit tests.
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6847
Summary:
I don't know if there is something more sinister going on
under the covers, but we have a couple of diffs that trigger:
Unhandled Exception ("BadMethodCallException")
Call to a member function getMetadata() on a non-object
when the diff page is handling its async render calls. One diff
in particular has multiple image adds and thus has a stack of of these
error dialogs to close.
This isn't a new regression, we just haven't gotten around to debugging
it until now (reported on 6/12)
One revision that triggers it has two diffs. If I show Base -> Diff 1
I don't hit the error. When I select Base -> Diff 2, or Diff 1 -> Diff
2, the error triggers.
I don't understand what this means, but this diff avoids the null object
reference that causes the exception.
Test Plan:
Load the offending diff, don't hit the error. The diff loads
the images that were added
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6851
Summary: Missed this when moving most MetaMTA responsibilities to the CLI. Show the correct command to get data rather than linking to a 404.
Test Plan: {F56733}
Reviewers: wez, btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6846
Summary:
D6660 accidentally allowed you to build Herald rules for commits that take action "Add to CC", but provided no implementation.
Someone at Facebook then wrote such a rule.
Fix forward since there's no real reason not to allow this.
Test Plan: Used `./scripts/repository/reparse.php --herald rXnnnn` to trigger rules. Observed rule trigger and subsequent subscription.
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6845
Summary:
Fixes T3781. The UI defaults to "Created" but the query defaults to "Modified". Make the two consistent.
In particular, an issue this fixes is that previously a `/differential/?authors=duck` page would show "Order: Created" but actually order by "Modified".
Test Plan: Visited `/differential/?authors=duck` and verified the revisions were ordered by creation date.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3781
Differential Revision: https://secure.phabricator.com/D6843
Summary: Ref T3780. Facebook has some environmental / itermittent stuff which would be easier to debug with host information on the setup issue screen.
Test Plan:
Checked both in-chrome and out-of-chrome versions of this screen, both looked reasonable.
{F56694}
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3780
Differential Revision: https://secure.phabricator.com/D6842
Summary:
Ref T3775 (discussion here). Ref T2625.
T3775 presents two problems:
# Existing tools which linked to `/differential/active/epriestley/` (that is, put a username in the URL) can't generate search links now.
# Humans can't edit the URL anymore, either.
I think (1) is an actual issue, and this fixes it. I think (2) is pretty fluff, and this doesn't really try to fix it, although it probably improves it.
The fix for (1) is:
- Provide a helper to read a parameter containing either a list of user PHIDs or a list of usernames, so `/?users[]=PHID-USER-xyz` (from a tokenizer) and `/?users=alincoln,htaft` (from an external program) are equivalent inputs.
- Rename all the form parameters to be more digestable (`authorPHIDs` -> `authors`). Almost all of them were in this form already anyway. This just gives us `?users=alincoln` instead of `userPHIDs=alincoln`.
- Inside ApplicationSearch, if a request has no query associated with it but does have query parameters, build a query from the request instead of issuing the user's default query. Basically, this means that `/differential/` runs the default query, while `/differential/?users=x` runs a custom query.
Test Plan: {F56612}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625, T3775
Differential Revision: https://secure.phabricator.com/D6840
Summary:
Ref T3772. The original version of D5451 had a very colorful version of this which felt a bit arbitrary, and we moved away from it after discussion, particularly [[ https://secure.phabricator.com/D5451#comment-8 | here (chad) ]] and [[ https://secure.phabricator.com/D5451#comment-14 | here (me) ]] and [[ https://secure.phabricator.com/D5451#comment-19 | here (chad again) ]].
The core of my objection was that status and priority to the viewer aren't the same: a "needs revision" revision that you authored is high priority (you need to revise it), but a "needs revision" revision that someone else authored is low priority (you're waiting on them to revise it). If we color by status, revisions in both high priority and low priority states will be colored red. We can instead color by viewer priority (blocking others = red, needs attention = orange, waiting on others = blue; or something), but that would be redundant (we already group by it, so you'd get big chunks of stuff with the same color and color would have no utility), confusing (in ungrouped views, the colors would not be self-explanatory) and weirdly inconsistent (different users would see objects having different colors).
I still think all this holds, but I also thought that "viewer priority" was enormously more important than "state", since I use the former frequently and the latter very rarely. From T3772, it sounds like some users use "state" a lot more than I do (i.e., they want to find "accepted" revisions within a "viewer priority" group like "Action Required"). This is a possible approach to that.
I think another issue was the heavy use of the color in the original; this restores a more conservative version of it which doesn't have as much weight. In particular:
- Revisions in the "Needs Review" state retain the default color, rather than orange.
- Revisions in the "Closed" state have the disabled effect.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3772
Differential Revision: https://secure.phabricator.com/D6839
Summary: Allows building Herald rules against committer, similar to author. Useful for monitoring cherry-picked commits.
Test Plan: Applied patch, restarted php-fpm and phd daemons to ensure code changes took effect. Added a new herald rule to trigger audit when committer was me. Cherry-picked someone else's commit (author=them, committer=me) and pushed to origin. Audit was triggered.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6838
Summary: Fixes T3486. I don't love how this looks -- maybe we could try different icons? Like white icons on a brighter red/yellow background?
Test Plan: {F56299}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3486
Differential Revision: https://secure.phabricator.com/D6833
Summary: When I swapped the views, I accidentally removed some controller -> view -> controller logic which is used to figure out which packages are highlighted. This code is a mess, but fix the feature for now and we can clean it up later.
Test Plan: {F56335}
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6835
Summary: Fixes T3773. By default, the `/users/` datasource excludes disabled users (since it doesn't make sense to assign them tasks or make them reviewers, for example). However, for ApplicationSearch it does make sense to look for objects, e.g., authored by a disabled user.
Test Plan: Searched for disabled users in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3773
Differential Revision: https://secure.phabricator.com/D6834
Summary:
D6335 has some unexpected side effects. This adds back the
where clause for the owned query. There may be other problems.
Test Plan:
Ran:
```
echo '{"query":"owned","guids":["myphid"]}' | arc --conduit-uri=https://myhost call-conduit differential.find
```
Reviewers: epriestley, dschleimer
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6832
Summary: Adds the new gradient to document views
Test Plan: Tested multiple pages in my sandbox in Phriction, UIExamples.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6827
Summary:
Ref T988. This is //extremely// rough looking in the UI, but gets most of the information we need into the right places.
The controller rendering code is super rough too, I'm going to break that up shortly.
- Add `needChildren()` to `DivinerAtomQuery`.
- Compose and organize class methods when rendering classes. The old Diviner was not smart enough to do this, so a class would only document methods which the class itself implemented, not methods inherited from parents. I'd like to show those too to provide a more complete understanding of how to use a class (but they'll be marked with "inherited" or somesuch). This code walks the "extends" list and builds all of the class methods, annotating them with where they are defined and where they are implemented.
- Coompose and organize "tasks". The old Diviner was not smart enough to do this, but I want to reduce the amount of duplicate/busy work involved in documenting subclasses. In particular, I want them to inherit "@task" declarations from parents so that class trees are more cohesive. They now do so.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6823
Summary: Ref T988. This was sort of hard-coded in one place and not done properly in another. Do it consistently.
Test Plan: Looked at atom list; looked at atom view. Saw "Article", "Class" rendered correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6821
Summary: Ref T988. Links up the "Declared:" property to point at a repository browser, if one exists.
Test Plan: Viewed a class document, saw a link, clicked it, got the definition.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6820
Summary:
Ref T988. This brings the class/interface atomizer over. A lot of parts of this are still varying degrees of very-rough, but most of the data ends up in approximatley the right place.
ALSO: PROGRESS BARS
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6817
Summary: Forms look like others, they do
Test Plan: Page reload, I see
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6826
Summary: Fixes T3763. All this junk needs some actual fixing at some point, but stop it from fataling.
Test Plan: Used `feed.query` with `view=text`. Before this patch, Phriction stories fataled. Now they render reasonably.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3763
Differential Revision: https://secure.phabricator.com/D6819
Summary:
Ref T988. Various improvements:
- Generate function documentation, mostly correctly.
- Raise some warnings about bad documentation.
- Allow `.book` files to exclude paths from generation.
- Add a book for technical docs.
- Exclude "ghosts" from common queries (atoms which used to exist, but no longer do, but which we want to keep the PHIDs around for in case they come back later).
This is a bit rough still, but puts us much closer to being able to get rid of the old Diviner.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6812
Summary: Some more callsites, let me know if you see others, I think think is 98% of them now.
Test Plan: tested each page
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6814
Summary:
This attempts some consistency in form layouts. Notably, they all now contain headers and are 16px off the sides and tops of pages. Also updated dialogs to the same look and feel. I think I got 98% of forms with this pass, but it's likely I missed some buried somewhere.
TODO: will take another pass as consolidating these colors and new gradients in another diff.
Test Plan: Played in my sandbox all week. Please play with it too and let me know how they feel.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6806
Summary: Fixes T2213
Test Plan: Updated a pholio mock description. Observed that when I first showed details there was a round trip made. Toggled show / hide noting no more trips made to server.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2213
Differential Revision: https://secure.phabricator.com/D6801
Summary: A serious business lost a bunch of serious business partners today because of this string, I assume.
Test Plan: Enabled serious mode, clicked button, was relieved to see no jokes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6799
Summary: Ref T3657. We currently try to generate a project crumb on the "Create Project" page, but fail. Paper that over until I can sort out T3657.
Test Plan: Loaded project create page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3657
Differential Revision: https://secure.phabricator.com/D6793
Summary: Ref T3663. Same as D6785, but for branches. No writes to this table yet.
Test Plan: Clicked "View History", got a blank but non-broken page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3663
Differential Revision: https://secure.phabricator.com/D6787
Summary:
We can get this out of PHIDType reasonably in all cases and simplify implementation here.
None of these translate correctly anyway so they're basically debugging/development strings.
Test Plan: `grep`, browsed some transactions
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6786
Summary: Ref T3663. There's no data recorded in this table yet, but add the UI and controller for it. Edits and such will eventually go here.
Test Plan: Clicked "View History" on a project, got an empty but non-broken page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3663
Differential Revision: https://secure.phabricator.com/D6785
Summary:
Ref T3663. This is a proto-transaction record which is obsoleted by real transactions. It has no UI, so I'm not bothering to retain/migrate the data since there's no regression.
Just get rid of it and all its writers. I'm keeping the table for now in case something crazy uses this somehow, so no data is actually destroyed.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3663
Differential Revision: https://secure.phabricator.com/D6784
Summary: This has two use sites and no special logic.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6783
Summary: Ref T3718. Releeph has a custom implementation of this exception; a more general version exists in CustomField. Use the more general one. Nothing catches the specific one.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6782
Summary: Ref T3092. This was obsoleted recently and has no more call/use sites.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3092
Differential Revision: https://secure.phabricator.com/D6779
Summary: Ref T3663. Does what it says on the tin.
Test Plan: Ran `storage upgrade`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3663
Differential Revision: https://secure.phabricator.com/D6778
Summary: Ref T3663. This is obsolete code which is used only in this migration, which Facebook has already performed and which isn't relevant for any other installs.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3663
Differential Revision: https://secure.phabricator.com/D6777
Summary: Ref T3092. Same deal as D6771, but for branches rather than projects.
Test Plan: {F54855}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T3092
Differential Revision: https://secure.phabricator.com/D6775
Summary:
Ref T3092.
Releeph's objects basically go like this:
- At the top level, we have Projects (like "www" or "libphutil")
- Each project has Branches (like "LATEST" or "v1.1.3")
- Each branch has Requests (like pull requests, e.g. "please merge commit X into branch Y (in project Z)")
Currently, there's no real "project detail" or "branch detail" page. Instead, we have a search results page for their contained objects. That is, the "project detail" page shows a list of branches in the project, using ApplicationSearch.
This means that operations like "edit" and "deactivate" are one level up, on the respective list pages.
Instead, move details onto the detail pages. This gives us more room for actions and information, and simplifies the list views.
Basically, these are "detail pages" where the object content is a search interface. We do something simliar to this in Phame right now, although it's messier there (no ApplicationSearch yet).
@chad, you might have some ideas here. Roughly, the design question is "How should we present an object's detail view when its content is really a search interface (Phame Blog for Posts, Releeph Project for Branches)?"
I think the simple approach I've taken here (see screenshot) gives us reasonable results, but overall it's something we haven't done much or done too much thinking about, I think.
Test Plan: {F54774}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3092
Differential Revision: https://secure.phabricator.com/D6771
Summary: Depends on D6769, removes 'dust' and uses a similar color background.
Test Plan: Review colors in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6772
Summary:
^\s+(['"])dust\1\s*=>\s*true,?\s*$\n
Test Plan: Looked through the diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6769
Summary: Fixes T2836
Test Plan: make a diff, get it approved, arc land, verify things okay. ask users on T2836 to try.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2836
Differential Revision: https://secure.phabricator.com/D6770
Summary:
This diff accomplishes this task by adding an arbitrary metadata store to PhabricatorObjectHandle. This seemed like it would be "necessary eventually"; for example if / when we decide we want to show images in these stories we'd need to add some more arbitrary data. A point of debate is this technique will yield the _current_ data and not the data at the time the transaction was originally made. I can see this being both desirable and non-desirable.
Otherwise, the best way to do this is to make a new transaction type specifically for create and store exactly what data we think we would need.
(and there's probably many other ways but they require much more work...)
Test Plan: viewed some pholio create stories and yes, they had the description showing.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3685
Differential Revision: https://secure.phabricator.com/D6767
Summary: Ref T2852. Asana is launching some kind of silent follow thing today; I don't know what the API is but it's probably something like this. I'll update this to actually make the right call once the call exists, this is mostly just a placeholder so I don't forget about it.
Test Plan: None yet, this API isn't documented or live and doesn't work yet so it can't be tested.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, moskov
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6740
Summary: Ref T3092. Fixes T3724. Use modern/flexible UI for these interfaces. Removes the ability to retarget an existing branch (you can just close it and open a new one if you made a mistake).
Test Plan: {F54437} {F54438}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3092, T3724
Differential Revision: https://secure.phabricator.com/D6765
Summary:
Releeph branch lists in project views have a bunch of custom UI right now; give them more standard UI and ApplicationSearch.
This drops a small piece of functionality: we now show only a total open request count instead of a detailed enumeration of each request status. I assume this is reasonable (that is, the important piece is "is there something to do on this branch?"), but we can muck with it if the more detailed status is important.
Test Plan: {F54344}
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3656
Differential Revision: https://secure.phabricator.com/D6764
Summary: Ref T2766. Does the integration via ApplicationTransactionsEditor. Only did addCC and Flag for proof of concept.
Test Plan: Made a rule to cc, made a rule to flag. They worked! (will attach screens to diff)
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2766
Differential Revision: https://secure.phabricator.com/D6766
Summary: somewhere along the line this broke. Before this patch we fail the visibility check since its based on Conpherence Participants which don't get created and attached until applyExternalEffects. Believe it or not, this was the least gross fix I could come up with; since the permission check is done SO early most other ideas I had involved creating a dummy participant object to pass the check then handling things for real later on... Ref T3723.
Test Plan: created a conpherence with myself - great success
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, Korvin, aran
Maniphest Tasks: T3723
Differential Revision: https://secure.phabricator.com/D6762
Summary:
Ref T3721. Releeph currently attempts to implement a flexible, field-driven search for branches, but it's building all of its own infrastructure and it ends up heading down some weird paths. In particular, it loads **every** request and then makes calls into fields to filter them. It also tries to be very very general, which isn't really necessary (for example, I think it's reasonable for us to assume that we won't let you disable the "requestor" field).
ApplicationSearch and CustomField provide more scalable approaches to this problem; move search on top of them. The query still ends up doing some filtering in-process, but it's now far more limited in scope and can be denormalized later.
Test Plan: {F54304}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3721
Differential Revision: https://secure.phabricator.com/D6758
Summary:
Ref T3718. This moves custom field rendering on the edit screen to PhabricatorCustomField and makes all the APIs conformant.
We still run through edit with both old-school and new-school sets of fields, because the actual editing isn't on the new stuff yet. That will happen in a diff or two.
Test Plan: Edited a request; intentionally introduced errors and verified the form behaved as expected.
Reviewers: btrahan, testuser1122344
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6756
Summary: Defaults hovercards off everywhere feed stories are shown. I tried to find where to put this in so /feed/ could display them, but got horribly lost and confused in SearchQueryLandView
Test Plan: turn hovercards on and off, inspect elements.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6757
Summary: Ref T3718. This is not used and does not seem particularly useful.
Test Plan: Grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6755
Summary: Currently, we check that the user can view and edit their own transaction, which is always true. Instead, check that they can view the object. I'll fix this with a more tailored check against the EDIT capability that's per-transaction later.
Test Plan: Applying no transactions no longer fatals with undefined `$xaction`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6754
Summary:
Ref T1703. Ref T3718. This introduces `PhabricatorCustomFieldAttachment`, which is just a fancy `array()`. The goal here is to simplify `PhabricatorCustomFieldInterface` as much as possible.
In particular, it can now use common infrastructure (`assertAttached()`) and is more difficult to get wrong.
Test Plan: Edited custom fields on profile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703, T3718
Differential Revision: https://secure.phabricator.com/D6752
Summary:
Ref T1703. Ref T3718. The `PhabricatorCustomFieldList` seems like a pretty good idea. Move more code into it to make it harder to get wrong.
Also the sequencing on old/new values for these transactions was a bit off; fix that up.
Test Plan: Edited standard and custom profile fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703, T3718
Differential Revision: https://secure.phabricator.com/D6751
Summary:
Fixes T3661. Ref T3718. This makes Releeph custom fields extend PhabricatorCustomField so we can start moving over other pieces of infrastructure (rendering, storage, etc) to run through the same pathways. It's roughly the minimum amount of work required to be able to move forward.
NOTE: This removes per-project custom field selectors. Fields are now configured for an entire install. My understanding is that Facebook does not use this feature, and modern field infrastructure has moved away from selectors.
Test Plan: Viewed and edited projects, branches, and requests in Releeph. Grepped for removed config. Grepped for `field_selector`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3661, T3718
Differential Revision: https://secure.phabricator.com/D6750
Summary:
Ref T1702. Ref T3718. There are a couple of things going on here:
**PhabricatorCustomFieldList**: I added `PhabricatorCustomFieldList`, which is just a convenience class for dealing with lists of fields. Often, current field code does something like this inline in a Controller:
foreach ($fields as $field) {
// do some junk
}
Often, that junk has some slightly subtle implications. Move all of it to `$list->doSomeJunk()` methods (like `appendFieldsToForm()`, `loadFieldsFromStorage()`) to reduce code duplication and prevent errors. This additionally moves an existing list-convenience method there, out of `PhabricatorPropertyListView`.
**PhabricatorUserConfiguredCustomFieldStorage**: Adds `PhabricatorUserConfiguredCustomFieldStorage` for storing custom field data (like "ICQ Handle", "Phone Number", "Desk", "Favorite Flower", etc).
**Configuration-Driven Custom Fields**: Previously, I was thinking about doing these with interfaces, but as I thought about it more I started to dislike that approach. Instead, I built proxies into `PhabricatorCustomField`. Basically, this means that fields (like a custom, configuration-driven "Favorite Flower" field) can just use some other Field to actually provide their implementation (like a "standard" field which knows how to render text areas). The previous approach would have involed subclasssing the "standard" field and implementing an interface, but that would mean that every application would have at least two "base" fields and generally just seemed bleh as I worked through it.
The cost of this approach is that we need a bunch of `proxy` junk in the base class, but that's a one-time cost and I think it simplifies all the implementations and makes them a lot less magical (e.g., all of the custom fields now extend the right base field classes).
**Fixed Some Bugs**: Some of this code hadn't really been run yet and had minor bugs.
Test Plan:
{F54240}
{F54241}
{F54242}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1702, T1703, T3718
Differential Revision: https://secure.phabricator.com/D6749
Summary: Ref T3656. Releeph denormalizes branch cut point identifiers into Branch objects, but this information isn't useful or used for sorting, filtering, or enforcing unique constraints. Instead, derive it via noramlized pathways from the `cutPointCommitPHID`.
Test Plan: Ran storage upgrade. Ran `releephwork.getbranch` and `releeph.getbranches`. Grepped for `cutPointCommitIdentifier`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3656
Differential Revision: https://secure.phabricator.com/D6636
Summary:
Fixes T3660. Releeph Projects currently have an unused one-to-one mapping to Phabricator projects. This isn't consistent with other applications and has no integrations or uses. Get rid of it.
NOTE: Waiting for signoff from @legneato on T3660 before pulling the trigger here.
Test Plan: Created and edited Releeph projects. Grepped for references to project ID; there are a dozen or so but they're all either Releeph projects or Arcanist projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3660
Differential Revision: https://secure.phabricator.com/D6635
Summary: Ref T3655. Depends on D6633. This removes the writes and the column.
Test Plan: Created a project, edited a project. Verified the table doesn't have any keys including this column.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3655
Differential Revision: https://secure.phabricator.com/D6634
Summary:
Ref T3655. ReleephProject currently has both `repositoryID` and `repositoryPHID`, which point to the same object and are reudundant. Get rid of all reads of `repositoryID`.
NOTE: This makes project loads depend on repository loads. The eventual rule here will be that you must be able to see a repository in order to see projects for that repository, which seems like a reasonable rule. We might need to tailor it more than this (e.g., if there are branch read permissions down the line) but this seems like a reasonable minimum.
Test Plan: Grepped for `repositoryID` in `releeph/`. Called `releeph.getbranches`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3655
Differential Revision: https://secure.phabricator.com/D6633
Summary:
Ref T1809. Provide ApplicationSearch to Flags and allow the user to select flags by color.
@chad might have some design feedback on my control.
Test Plan: {F54131}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D6747
Summary:
Ref T1809. Ref T603. Ref T3599. Makes flags policy aware.
This change reduces the utility of flag search/browse; the next change will switch it to ApplicationSearch to restore utility. Representing all that ordering in terms of cursor paging is also a giant pain.
Test Plan: Viewed Differential, Flags, etc. Grepped for all PhabricatorFlagQuery callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T1809, T3599
Differential Revision: https://secure.phabricator.com/D6746
