Summary: Fixes T2210. Recently, we require unique keys on menu items, but it's currently possible in Maniphest to save the same custom query under multiple names. Avoid exploding in this case (we'll hide the duplicates). This isn't a great fix, but makes Maniphest usable again.
Test Plan: Saved the same query twice, laoded page, got exception, applied patch, loaded page, saw duplicate query stripped.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2210
Differential Revision: https://secure.phabricator.com/D4247
Summary: We use "D<id>" for revisions.
Test Plan: Looked at revision.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4258
Summary: 'cuz new fluid layouts require the westerlyness. Looks like D4126 started the N and W implementation but didn't finish it...? note I had to do the shifting of the 5 pixels in javascript; using the CSS didn't work for me in chrome.
Test Plan: uiexample, and hoping it goes well when deployed in prod for differential case
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2211
Differential Revision: https://secure.phabricator.com/D4257
Summary: This is used in every other view.
Test Plan: Browsed around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4248
Summary:
I messed up D4244 (and didn't test it properly) - this change should load
the raw content so we can use it.
Test Plan: use the conduit console and actually test the paste conduit methods
Reviewers: vrana, epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4245
Summary:
The caching introduced by rP7e37eb48273eef87e6e6811703fb5d85a3e07a81
broke the use of PhabricatorPaste::getContent() for forking/editing
pastes and downloading pastes with arc paste. I think this fixes all
the appropriate callsites.
Test Plan: fork a paste in the web ui
Reviewers: vrana, epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4244
Summary:
See f5c2a2ab4b (commitcomment-2333247)
Copy of working implementation from PHPMailerLite.
Also expose the SSL/TLS options.
Test Plan: Switched to this mailer, configured Gmail SMTP, sent email. Verified email arrived intact.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, mbeck
Differential Revision: https://secure.phabricator.com/D4239
Summary:
- Original command is in SSH_ORIGINAL_COMMAND, not normal argv.
- Use PhutilShellLexer to parse it.
- Fix a protocol encoding issue with ConduitSSHWorkflow. I think I'm going to make this protocol accept multiple commands anyway because SSH pipes are crazy expensive to build (even locally, they're ~300ms).
Test Plan: With other changes, successfully executed "arc list --conduit-uri=ssh://localhost:2222".
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D4232
Summary:
- Build "sshd-auth" (for authentication) and "sshd-exec" (for command execution) binaries. These are callable by "sshd-vcs", located [[https://github.com/epriestley/sshd-vcs | in my account on GitHub]]. They are based on precursors [[https://github.com/epriestley/sshd-vcs-glue | here on GitHub]] which I deployed for TenXer about a year ago, so I have some confidence they at least basically work.
- The problem this solves is that normally every user would need an account on a machine to connect to it, and/or their public keys would all need to be listed in `~/.authorized_keys`. This is a big pain in most installs. Software like Gitosis/Gitolite solve this problem by giving you an easy way to add public keys to `~/.authorized_keys`, but this is pretty gross.
- Roughly, instead of looking in `~/.authorized_keys` when a user connects, the patched sshd instead runs `echo <public key> | sshd-auth`. The `sshd-auth` script looks up the public key and authorizes the matching user, if they exist. It also forces sshd to run `sshd-exec` instead of a normal shell.
- `sshd-exec` receives the authenticated user and any command which was passed to ssh (like `git receive-pack`) and can route them appropriately.
- Overall, this permits a single account to be set up on a server which all Phabricator users can connect to without any extra work, and which can safely execute commands and apply appropriate permissions, and disable users when they are disabled in Phabricator and all that stuff.
- Build out "sshd-exec" to do more thorough checks and setup, and delegate command execution to Workflows (they now exist, and did not when I originally built this stuff).
- Convert @btrahan's conduit API script into a workflow and slightly simplify it (ConduitCall did not exist at the time it was written).
The next steps here on the Repository side are to implement Workflows for Git, SVN and HG wire protocols. These will mostly just proxy the protocols, but also need to enforce permissions. So the approach will basically be:
- Implement workflows for stuff like `git receive-pack`.
- These workflows will implement enough of the underlying protocol to determine what resource the user is trying to access, and whether they want to read or write it.
- They'll then do a permissons check, and kick the user out if they don't have permission to do whatever they are trying to do.
- If the user does have permission, we just proxy the rest of the transaction.
Next steps on the Conduit side are more simple:
- Make ConduitClient understand "ssh://" URLs.
Test Plan: Ran `sshd-exec --phabricator-ssh-user epriestley conduit differential.query`, etc. This will get a more comprehensive test once I set up sshd-vcs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T550
Differential Revision: https://secure.phabricator.com/D4229
Summary: in the re-factor quest this got broken. Turns out NewLines (aka $this->new in the parser) gets updated right up until the very end for showing text changes so instead pass over the total line count to the renderer to get this codepath right.
Test Plan: showed content of some generated files in diffusion and all was well
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2204
Differential Revision: https://secure.phabricator.com/D4237
Summary:
Because of the way PhabricatorOwnersPackage works, the code to save package
changes when parsing commit changes was raising a few undefined index errors,
and was throwing an exception trying to call a method on null (because not
all of the phids related to the package had their handles loaded). This fix
doesn't load the missing handles, it just avoids trying to use them.
Test Plan:
./scripts/repository/reparse.php on a commit with path changes that triggered
a package change
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4236
Summary:
Load the data for daemon worker tasks when viewing them, and present
the information in a useful way. This defaults to printing the json data,
but for some classes of worker it will also link to the corresponding
object, to make debugging problems with workers easier.
Test Plan:
load /daemon/task/NNN for a CommitParserWorker and a MetaMTAWorker, and
see the addition of a data field with useful content and link.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4226
Summary: This is probably not the most useful app to have work on mobile, but get the log view to do something fairly sensible.
Test Plan: Looked at all Drydock views in mobile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4224
Test Plan: Called it on a diff with postponed linters and no messages.
Reviewers: mgummelt, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4223
Summary: its a bit confusing but "newer" posts are the "previous" page and "older" posts are the "next" page. this is because newer posts are those with higher ids. also make the title be the title of the post if we have an actual post.
Test Plan: set page limit to 5 and got somewhat sensical results (note this pagination seems to break with my test data set where there's fun gaps in the contiguity of the ids in a given blog) viewed an actual post and noted the page title was the post title
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4222
Summary: The logs bits still need some work but add crumbs/lists to everything else. Also build a propery DrydockResourceQuery.
Test Plan: Looked at lease list/detail; resource list/detail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4221
Summary: arc incorrectly passes a "user" parameter to differential.createrevision (long ago, we respected it, I think). After D4191 this fatals. Provide a stub call until the next version bump.
Test Plan: inspection
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4220
Summary: Minor updates to Drydock things to make them work better. In particular, after this patch working copies are correctly allocated or reused.
Test Plan: Ran "reparse.php --harbormaster <derp derp>", saw reuse of working copies when unleased resources were avilable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4216
Summary:
This is very preliminary and doesn't actually do anything useful. In theory, it uses Drydock to check out a working copy and run tests. In practice, it's not actually capable of running any of our tests (because of complicated interdependency stuff), but does check out a working copy and //try// to run tests there.
Adds various sorts of utility methods to various things as well.
Test Plan: Ran `reparse.php --harbormaster --trace <commit>`, observed attempt to run tests via Drydock.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015, T1049
Differential Revision: https://secure.phabricator.com/D4215
Summary: I plan to use this in Arcanist.
Test Plan:
$ echo '{}' | arc call-conduit x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4192
Summary:
Modernizes file uploads. In particular:
- Adds a mobile menu, with an "Upload File" item.
- Adds crumbs to the list view, detail view and upload view.
- Adds "Upload File" action to crumbs.
- Moves upload file to a separate page.
- Removes the combined upload file + recent files page.
- Makes upload file use a normal file control by default (works on mobile).
- Home page, file list and file upload page are now global drop targets which accept files dropped anywhere on them. Dragging a file into the window shows a mask and an instructional message.
- User education on this is a little weak but I think that's a big can of worms?
- Fixes a bug where dropping multiple files into a Remarkup text area produced bad results (resolves T2190).
T879 is related, although it's specifically about Maniphest. I've declined to make global drop targets yet there because there are multiple drop targets on the page with different meanings. That UI needs updating in general.
@chad, do we have an "upload" icon (counterpart to "download")?
Test Plan: Uploaded files in Maniphest, Differential, Files, and from Home. Dragged and dropped multiple files into Differential. Used crumbs, mobile.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2190
Differential Revision: https://secure.phabricator.com/D4200
Summary:
D4188 adds a preview of Paste contents to the list, but gets slow for large lists if you have Pygments installed since it has to spend ~200ms/item highlighting them. Instead, cache the highlighted output.
- Adds a Paste highlighting cache. This uses RemarkupCache because it has automatic GC and we don't have a more generalized cache for the moment.
- Uses the Paste cache on paste list and paste detail.
- Adds a little padding to the summary.
- Adds "..." if there's more content.
- Adds line count and language, if available. These are hidden on Mobile.
- Nothing actually uses needRawContent() but I left it there since it doesn't hurt anything and is used internally (I thought the detail view did, but it uses the file content directly, and must for security reasons).
Test Plan:
{F27710}
- Profiled paste list, saw good performance and few service calls.
- Viewed paste.
- Viewed raw paste content.
Reviewers: codeblock, btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4204
Summary:
Use modern elements for file detail view.
For the "Technical Details", maybe we should implement a disclosure triangle element? I'd guess there are other cases we could use it.
This makes two small practical changes:
- We show "Delete File" even if you can't delete it; I'm going to align this properly with CAN_EDIT shortly.
- We no longer show the feature discovery hint about using "arc download".
Test Plan:
{F27179}
{F27180}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4199
Summary: Update to new UI stuff, prepare for mobile.
Test Plan: {F27167}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4198
Summary:
This adds a "body" field to `PhabricatorObjectItemView` which lets you optionally
add more information to the list view. It then uses this new field to show
samples of pastes in the paste list view.
Test Plan:
{F27147}
{F27148}
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4188
Summary:
Add a `setViewer()` so that PHID loading doesn't throw.
Also remove a period from a button because none of the others have one.
Test Plan: Saved a comment successfully.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4202
Summary: Permit the forcible release of Drydock leases. The implementation isn't very exciting for now.
Test Plan: Released leases via web and CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4181
Summary: Add a WIKI block to the switch in PhabricatorObjectHandleData->loadObjects().
Test Plan: Went to /feed/ and saw my wiki edit stories.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4180
Summary: See discussion in T2014. Aligns this element more closely with @chad's `frame_v3.psd` mock, and implements the icon/label element. Removes "details".
Test Plan: {F27062} {F27063} {F27064} {F27065}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D4179
Summary: most awesome is that differential-primary-pane no longer has a place in diffusion. less awesome is fixing the zebra striping on differential "Local Commits" view and making the font size of one of the table headers match the others.
Test Plan: looks good!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4177
Summary: we don't always have a diff so instead set an explicit title in the controller.
Test Plan: no more fatals. grepped carefully for every call site and tested them all
Summary: basically made the header elements for the individual panes and cleaned up the panes to make it look okay. tried to copy colors from @chad 's mocks.
Test Plan: looks good to me
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D4174
Summary:
If something that doesn't belong to any field appears in the commit message
below the differential revision field, it gets included as part of the
value for the field, which can mess up parsing.
Test Plan:
called differential.parsecommitmessage on a commit whose differential
revision field wasn't being parsed earlier (it had a line of dashes two
lines below the Differential Revision: line).
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4171
Summary:
Right now, Drydock gives out multiple leases to the same working copy and gives out leases to working copies with repository "P" in them when the user requested some other repository.
Add two callbacks:
- `canAllocateLease()` - allows a blueprint to reject a lease on a resource because of a fundamental incompatibility, like "it's a working copy with Phabricator in it, but the lease wants a working copy with Javelin in it".
- `shouldAllocateLease()` - allows a blueprint to reject a lease on a resource because of resource limits, like "only one active lease can own a working copy at a time".
Also cleaned up various other things.
Test Plan:
After implementing the callbacks, Drydock has the correct behavior:
- It gives multiple leases on `localhost`, but only one lease per working-copy resource.
- It does not grant leases on resources with repository X to requests for repository Y.
Ran `bin/drydock lease --type working-copy --repositoryID 12` and similar repeatedly and verified results in the web console.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4166
Summary:
- Remove "Diffusion" crumb (redundant with application icon)
- Put "All Repositories" in its place on the landing page.
- Render the repository crumb as "rX" instead of "X Repository".
Test Plan: Looked at various Diffusion pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4173
Summary:
upgrades are CrumbsView, HeaderView, PropertyListView, and ActionListView. I had to modify CrumbsView stuff a bit to handle the "advanced" diffusion crumbs.
Quirks fixed include making file tree view show up in diffusion, the page not have extra space when the file tree is hidden, links no longer breaking once you visit files (since without the change the files always got "/" appended and thus 404'd), and a differential quirk where it read "next step:" and that colon is a no no,
Test Plan: played around in diffusion and differential
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T2048, T2178
Differential Revision: https://secure.phabricator.com/D4169
Summary:
Clicking "show details" of a task description change in Maniphest currently throws an exception about the markup engine.
Since we don't actually need the engine an alternate fix would be "if ($this->markupEngine) { $renderer->setMarkupEngine($this->markupEngine); }" but we have one at the ready so just provide it. This should become part of the Transactions stuff anyway.
Test Plan: Clicked "show details".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4167
Summary:
This adds a configuration option for repositories to be marked as managed
by phabricator, which is then used by the pullLocal daemon to try to recover
from some errors it runs into.
Test Plan: Set a bogus uri for a repo, saw that the pullLocal daemon fixed it.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3680
