Summary: Main plan is to give conversations in Conpherence or Durable Column a different, lighter, chatty feel like Phriction.
Test Plan:
Tested a couple of threads and remarkup styles.
{F278086}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11562
Summary: Fixes T6819. This isn't as useful as you might think and has one horribly buggy behavior - if you edit an object which has a description and a projects field, you can be unable to remove the associated project as the automagic association from the description kicks in. Further, since we've added the ability for applications to create multiple email addresses AND herald can react to those emails - say by programmatically adding projects - the known needs for this feature are basically 0. If this proves to be false we can maybe add some other syntax for these mentions - see T6819 for ideas / discussion.
Test Plan: removed a project from a maniphest task while still mentioning it in the description and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6819
Differential Revision: https://secure.phabricator.com/D11573
Summary: Fixes T3404 (post D11565), fixes T5952. This infrastructure has been getting deployed against Maniphest and its time to get these other two applications going on it.
Test Plan: created an email address for paste and used `./bin/mail receive-test` ; a paste was successfully created
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952, T3404
Differential Revision: https://secure.phabricator.com/D11570
Summary: Ref T3404. The only mildly sketchy bit is these codepaths all load the application email directly, by-passing privacy. I think this is necessary because not getting to see an application doesn't mean you should be able to break the application by registering a colliding email address.
Test Plan:
Tried to add a registered application email to a user account via the web ui and got a pretty error.
Ran unit tests.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404
Differential Revision: https://secure.phabricator.com/D11565
Summary: due to typehints, passing null is going to barf here. Ref D11564, ref T5039.
Test Plan: made an edit to a task from the web ui and it didnt fatal
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11571
Summary:
Hit this locally, with an error like:
> Version <empty string> is older than 1.9, the minimum supported version.
(Where `<empty string>` was just the empty string, not literally the text `<empty string>`.)
Be more careful about parsing versions, and parse the newer string.
Test Plan: Got "unknown version" with intentionally-broken test data, then clean readout.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11558
Summary: Fix 'No Conpherences' layout, add 'Recent' label to list.
Test Plan: test with and without a list of threads.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11569
Summary:
Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???"
Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective.
Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11564
Summary: Fixes T7078. Adds a `./bin/storage shell` command which passes through to a MySQL shell. This is slightly more convenient than running `mysql` manually.
Test Plan: Ran `./bin/storage shell` and got a MySQL shell.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7078
Differential Revision: https://secure.phabricator.com/D11548
Summary: This adds a check to make sure the credential exists when loading it in the Drydock SSH interface. This effectively turns a fatal error (calling a method on a non-object) into a catchable exception.
Test Plan: Had a badly configured resource, saw the exception appear instead of daemon fataling.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11530
Summary: Fixes T7007. Using `%B` permits non-UTF8 data to be appended to Harbormaster build logs. Since we're not really in control of the processes Harbormaster is running remotely, and since they may output invalid UTF8 data, we should store the invalid data instead of failing the build (due to UTF8 exception).
Test Plan: @epriestley said this was the right fix, though I haven't tested it on our production system which actually exhibits the issue yet.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7007
Differential Revision: https://secure.phabricator.com/D11532
Summary: Fixes T7034. Like HTTP, proxy requests to the correct host if a repository has an Almanac service host.
Test Plan: Ran VCS requests through the proxy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034
Differential Revision: https://secure.phabricator.com/D11543
Summary: Ref T5039. This will be necessary for Herald integration so users can make rules like "if app email is one of x, y, or z add projects foo, bar, and metallica." I think its best to do an actual typeahead here -- users select full email addresses -- rather than support prefix, suffix, etc stuff on the email address. I think the latter approach would yield lots of confusion, as well as prevent us from (more) easily providing diagnostic tools about what happened when and why.
Test Plan: hacked a maniphest tokenizer to use this new datasource and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11546
Summary: In Maniphest, we say "X closed <task> by committing <commit>". In Differential, we currently say "X closed <revision> by commit <commit>", which sounds nongrammatical to me.
Test Plan: grammar'd
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11544
Summary: Ref T5952. This adds support for a "default author" and deploys it on Maniphest.
Test Plan: used augmented (by this diff) bin/mail receive-test to test creation via an application email with a default author configured and no author specified. a task was created with the author as the default author i configured.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952
Differential Revision: https://secure.phabricator.com/D11446
Summary:
Ref T7034.
In a cluster environment, when a user connects with a VCS request over SSH (like `git pull`), the receiving server may need to proxy it to a server which can actually satisfy the request.
In order to proxy the request, we need to know which repository the user is interested in accessing.
Split the SSH workflow into two steps:
# First, identify the repository.
# Then, execute the operation.
In the future, this will allow us to put a possible "proxy the whole thing somewhere else" step in the middle, mirroring the behavior of Conduit.
This is trivially easy in `git` and `hg`. Both identify the repository on the commmand line.
This is fiendishly complex in `svn`, for the same reasons that hosting SVN was hard in the first place. Specifically:
- The client doesn't tell us what it's after.
- To get it to tell us, we have to send it a server capabilities string //first//.
- We can't just start an `svnserve` process and read the repository out after a little while, because we may need to proxy the request once we figure out the repository.
- We can't consume the client protocol frame that tells us what the client wants, because when we start the real server request it won't know what the client is after if it never receives that frame.
- On the other hand, we must consume the second copy of the server protocol frame that would be sent to the client, or they'll get two "HELLO" messages and not know what to do.
The approach here is straightforward, but the implementation is not trivial. Roughly:
- Start `svnserve`, read the "hello" frame from it.
- Kill `svnserve`.
- Send the "hello" to the client.
- Wait for the client to send us "I want repository X".
- Save the message it sent us in the "peekBuffer".
- Return "this is a request for repository X", so we can proxy it.
Then, to continue the request:
- Start the real `svnserve`.
- Read the "hello" frame from it and throw it away.
- Write the data in the "peekBuffer" to it, as though we'd just received it from the client.
- State of the world is normal again, so we can continue.
Also fixed some other issues:
- SVN could choke if `repository.default-local-path` contained extra slashes.
- PHP might emit some complaints when executing the commit hook; silence those.
Test Plan: Pushed and pulled repositories in SVN, Mercurial and Git.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034
Differential Revision: https://secure.phabricator.com/D11541
Summary: Add a setBorder call to CrumbsView to be more deliberate when a border is drawn. Could not find any CSS hacks to set it conditionally CSS.
Test Plan: Browsed every application that called crumbs and make a design decision. Also fixed a few bad layouts.
Reviewers: btrahan, epriestley
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11533
Summary: Swaps out AphrontPanels for ObjectBoxes. I'd like to start reducing the floating object lists around the site for consistency. Also, these should provide more items above the fold.
Test Plan:
Test on my local homepage. Built a fake welcome.html too, though I think that's deprecated.
{F277020}
{F277021}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11529
Summary:
Ref T2086. Ref T7014. With the persistent column, there is significant value in retaining chrome state through navigation events, because the user may have a lot of state in the chat window (scroll position, text selection, room juggling, partially entered text, etc). We can do this by capturing navigation events and faking them with Javascript.
(This can also improve performance, albeit slightly, and I believe there are better approaches to tackle performance any problems which exist with the chrome in many cases).
At Facebook, this system was "Photostream" in photos and then "Quickling" in general, and the technical cost of the system was //staggering//. I am loathe to pursue it again. However:
- Browsers are less junky now, and we target a smaller set of browsers. A large part of the technical cost of Quickling was the high complexity of emulating nagivation events in IE, where we needed to navigate a hidden iframe to make history entries. All desktop browsers which we might want to use this system on support the History API (although this prototype does not yet implement it).
- Javelin and Phabricator's architecture are much cleaner than Facebook's was. A large part of the technical cost of Quickling was inconsistency, inlined `onclick` handlers, and general lack of coordination and abstraction. We will have //some// of this, but "correctly written" behaviors are mostly immune to it by design, and many of Javelin's architectural decisions were influenced by desire to avoid issues we encountered building this stuff for Facebook.
- Some of the primitives which Quickling required (like loading resources over Ajax) have existed in a stable state in our codebase for a year or more, and adoption of these primitives was trivial and uneventful (vs a huge production at Facebook).
- My hubris is bolstered by recent success with WebSockets and JX.Scrollbar, both of which I would have assessed as infeasibly complex to develop in this project a few years ago.
To these points, the developer cost to prototype Photostream was several weeks; the developer cost to prototype this was a bit less than an hour. It is plausible to me that implementing and maintaining this system really will be hundreds of times less complex than it was at Facebook.
Test Plan:
My plan for this and D11497 is:
- Get them in master.
- Some secret key / relatively-hidden preference activates the column.
- Quicksand activates //only// when the column is open.
- We can use column + quicksand for a long period of time (i.e., over the course of Conpherence v2 development) and hammer out the long tail of issues.
- When it derps up, you just hide the column and you're good to go.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2086, T7014
Differential Revision: https://secure.phabricator.com/D11507
Summary:
Fixes T7019. In a cluster environment, pushes currently fail because the commit hook can't identify the instance.
For web processes, the hostname identifies the instance -- but we don't have a hostname in the hook.
For CLI processes, the environment identifies the instance -- but we don't have an environment in the hook under SVN.
Promote the instance identifier into the upstream and pack/unpack it explicitly for hooks. This is probably not useful for anyone but us, but the amount of special-purpose code we're introducing is very small.
I poked at trying to do this in a more general way, but:
- We MUST know this BEFORE we run code, so the normal subclassing stuff is useless.
- I couldn't come up with any other parameter which might ever be useful to pass in.
Test Plan: Used `git push` to push code through proxied HTTP, got a clean push.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11495
Summary:
Ref T7019. When we receive a `git clone https://` (or `git push` on HTTP/S), and the repository is not local, proxy the request to the appropriate service.
This has scalability limits, but they are not more severe than the existing limits (T4369) and are about as abstracted as we can get them.
This doesn't fully work in a Phacility context because the commit hook does not know which instance it is running in, but that problem is not unique to HTTP.
Test Plan:
- Pushed and pulled a Git repo via proxy.
- Pulled a Git repo normally.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11494
Summary:
Ref T7019. Adds a new response which can proxy an HTTP request and pass the result through.
This is grossly inefficient for the same reasons as HTTP hosting is generally inefficient right now (T4369). This stuff is fixable but not trivial.
Test Plan: Replaced home page with a proxy to `example.org`, used Charles to view headers, saw the page headers and content proxy with an X-Phabricator-Proxied header.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11492
Summary:
Ref T7019. Ref T7034. In both proxying cases, we want to proxy the request but can not do so over Conduit.
Split the URI resolution apart from Conduit client construction so we can just pull an SSH or HTTP/S URI out of the repository without getting an entire Conduit client.
Test Plan: Browsed around a service-hosted repository. This diff has no behavioral changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034, T7019
Differential Revision: https://secure.phabricator.com/D11491
Summary:
Ref T6881. This roughs in the major objects, support classes, and controllers.
- Show subscriptions on account detail.
- Browse all account subscriptions.
- Link to active subsciptions from merchant detail.
Test Plan: Clicked around in the UI. There's no way to create subscriptions yet, so I basically just kicked the tires on this. I probably missed a few things that I'll clean up in followups.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11482
Summary:
Ref T7055. Apparently we just never had one? I feel like I'm crazy. But I can't find any trace in the logs.
I'm actually not 100% sold on this being better because it's a color glyph on OSX and those feel a little out of place / tacky to me compared to the black-and-white ones. So I'd be fine with just leaving it off, too. Clearly not important if no one noticed it until I caught it in T7055.
Test Plan: {F276917}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7055
Differential Revision: https://secure.phabricator.com/D11524
Summary: Unused at this point
Test Plan: Grep
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11506
Summary: Adds in the sidenav
Test Plan: Click on sidenav, see it persist
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11526
Summary:
Ref T7014. This is very rough and not hooked up to anything, but gets a couple of the layout pieces in place so we can (a) see that it looks like it'll kinda work; (b) look for problematic interactions and (c) you can fix my mangling of your design.
NOTE: Press "\" to toggle the column.
Test Plan:
Feels pretty good to me?
{F275722}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11497
Summary: Fixes T7050. I got the regexp slightly wrong and didn't catch it because it works fine on modern MySQL.
Test Plan: `arc unit --everything` still passes.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7050
Differential Revision: https://secure.phabricator.com/D11522
Summary: With the new magic controller switcher, these links are needed.
Test Plan: Look at list of Projects
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11510
Summary: Adds it back
Test Plan: Give token, view story
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11498
Summary: Removes the 1x application icons, and uses the fonticons instead. Feed was only known location.
Test Plan:
feed, dashboards, grep for use
{F275636}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11496
Summary: Fixes T7033. When we've reframed the main page content we need to scroll relative to the containing frame, not relative to the window.
Test Plan:
In Safari, Chrome and Firefox, used j/k/J/K keys to navigate diff content.
Tried some other scroll-based beahviors, like jump-to-anchors.
(It looks like the highlighting reticle got slightly derped a while ago, but it's still functional, so I didn't mess with it.)
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7033
Differential Revision: https://secure.phabricator.com/D11490
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary:
Ref T2783. I think this served two purposes:
- Improving performance in cases where we "know" a repository is local.
- Preventing loops.
It is now obsolete:
- After D11476, refs can almost always resolve on a fast path.
- As T2783 moves forward, we can usually no longer know when a repository is local without actually looking it up -- almost everything is allowed to run anywhere.
- The cluster behavior in D11475 now prevents loops.
Test Plan: `grep`, browsed around. This didn't really do much of anything anymore.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11477
Summary:
Ref T2783. With service-oriented calls, we take a larger performacne hit than necessary resolving refs.
Instead of resolving refs over the wire, try to resolve them from the database first. This can resolve almost all refs (commit hashes, branch and tag names).
This can't resolve weird refs like `master~50`, and obviously can't resolve invalid refs. In those cases we'll go back to the old logic, call `diffusion.resolverefs`, and end up with the right result.
Test Plan:
- Browsed repositories in Diffusion.
- Verified that service repositories no longer make unnecessary `diffusion.resolverefs` calls for common refs (branch names, commit hashes).
- Resolved refs like `master~50`, saw call to underlying VCS and correct result.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11476
Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.
This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.
In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).
Test Plan:
- With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
- With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11475
Summary: Ref T7020. I need this elsewhere, and it's relatively internal anyway.
Test Plan: Browsed around my local, cluster-configured install and saw everything working fine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11474
Summary:
Ref T7014. With a mouse plugged in, multi-panel UIs are pretty hideous on OSX. This is somewhat offputting for me in Conpherence, and really jumps out at me with the new column mocks in T7014.
Sites like Twitch and Facebook approach this by emulating the touchpad scrollbar to achieve a more aesthetic UI. Use a similar approach.
This:
- Replaces the main scrollbar with a prettier fake one.
- This prepares the standard page frame for a persistent chat column.
Test Plan:
- Seems to work properly on OSX, Chrome and Firefox. Haven't tested on IE; my Windows setup is pretty iffy at the moment.
- Tried Conpherence.
- Tried Workboards.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11472
Summary: Fixes T7021. When I moved around all the timeline stuff I guess I didn't find this "corner" case, which is wildly common in the post-commit review workflow that we don't use.
Test Plan: pre-patch I could reproduce the issue and post patch I could not. The reproduction case is to have a commit with inline comments and then enough subsequent comments to have a "show older" UI. clicking "show older" now works!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7021
Differential Revision: https://secure.phabricator.com/D11479
Summary: Fixes T7011. Recent refactoring here caused us to begin ignoring URI parameters like `commit`. Most controllers take parameters as a `dblob`, which was still parsed properly.
Test Plan:
- Editing different commits actually edits the desired commits.
- Browsed around some `dblob` pages and verified they still work properly.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7011
Differential Revision: https://secure.phabricator.com/D11473
Summary:
One advantage I wanted to get out of T1191 is automated rebuilds of `quickstart.sql`. If they don't actually work, I'd like to know sooner rather than later. We haven't rebuilt in a couple months, so give it a shot.
Ran into two issues:
- Some very old patches specify overlong keys which don't work if your default charsets are utf8mb4. Shorten these. No real users have applied these in a very long time.
- Some gymnastics around `corpus` for the new Conpherence search index.
Test Plan:
- Ran `arc unit --everything`, got clean results.
- Cost to do a storage upgrade on an empty namespace dropped from ~4s to ~3s.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11454
Summary: I got these wrong and the test didn't trigger for some reason that I haven't looked into.
Test Plan: `arc unit --everything`
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11453
Summary:
Ref T5833. In some cases, we need to know if an Almanac device is the localhost or not, so we can either handle or forward the request.
To accomplish this, write a device ID when running `bin/almanac register`.
Using `--allow-key-reuse` and `--identify-as`, multiple devices are permitted to //authenticate// as one device but //identify// as different devices. In the Phacility cluster, this allows all the `repoXXX` machines to have one keypair (making key management much easier) but still work as separate devices. This is an advanced feature; normal installs with 1-3 hosts would just generate a key + device per host and identify/authenticate as the same device.
Test Plan: Ran commands with lots of flags like `PHACILITY_INSTANCE=local sudo -E ./bin/almanac register --device daemon.phacility.net --private-key ~/dev/core/conf/keys/daemon.key --force --allow-key-reuse --identify-as local001.phacility.net`. Got a good result from `AlmanacKeys::getDeviceID()` afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D11452
Summary: Fixes T6890. This doesn't feel like a perfect solution, but I can't think of any cases in which this will produce the wrong result either.
Test Plan: Ran `./bin/diviner generate` and checked the generated documentation for `PhabricatorCommonPasswords::loadWordlist()`. The return type was corrected shown as `map<string, bool>`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6890
Differential Revision: https://secure.phabricator.com/D11469
Summary: As suggested in T6950, add the method description to the response from `conduit.query`.
Test Plan: Called `echo '{}' | arc call-conduit conduit.query` and verified that the response contained the method description.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11467
Summary: Fixes T6950. Adds the return type of Conduit API methods to the `conduit.query` call.
Test Plan: Called `echo '{}' | arc call-conduit conduit.query` and verified that the return types were present in the response.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6950
Differential Revision: https://secure.phabricator.com/D11466
Summary:
Fixes T6858. We shouldn't create mentions for dependent diffs.
NOTE: This won't fix the issue for existing revisions (which have the mentions edge), but I think that this is harmless.
Test Plan: Added `Depends on Dxxx` to a differential summary. Saw a `josh added a dependent revision` transaction, but no explicit mention.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6858
Differential Revision: https://secure.phabricator.com/D11460
Summary:
We have to do some garbage nonsense to write database backups right now, see T6996.
When storage isn't initialized, we previously ended up with this message gzipped in a file and an empty error. Make the behavior slightly more tolerable.
Test Plan: Saw a meaningful error after trying to back up an uninitialized database.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11449
Summary: Ref T5833. This was using the wrong constant, so we weren't validating property.
Test Plan: Tried to create a nameless network and correctly got an error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D11447
Summary: Fixes T6989. Basically return a nice dialogue like we do for "NoEffect" transactions. This is a little prettier than the other dialogue was. Also, stop adding TYPE_EDGE as a transaction type as we end up having it 2x, which then makes the error get validated 2x.
Test Plan: tried to add myself as a reviewer and got a nice error message.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6989
Differential Revision: https://secure.phabricator.com/D11448
Summary:
Ref T6881. I tried to cheat here by not implementing this, but we need it for destroying triggers directly with `bin/remove destroy`, since that needs to load them by PHID.
So, cheat slightly less. Implement PolicyAware but not CursorPagedPolicyAware.
Test Plan:
- Used `bin/remove destroy` to destroy a trigger by PHID.
- Browsed daemon console.
- Ran trigger daemon.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11445
Summary:
Ref T6881. This makes it easier to fire a trigger and make sure it works properly. You can use the `--now` flag to travel through time, and test scheduling conditions with `--last` and `--next`. It will tell you when the trigger would reschedule.
Better than waiting 24 hours to see if things work.
Test Plan: Fired some backups, got useful output which made me think my code probably works correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11438
Summary: Ref T6881. This is useful to show a "Next backup: 2:30 AM" sort of thing without requring callers to know how triggers work internally.
Test Plan: Showed that kind of thing in Instances.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11437
Summary:
Ref T6881. By design, the EXECUTION order only selects tasks which have been scheduled (since it performs a JOIN). This is inconsistent with other queries and problematic for withID/withPHID queries which may want to select an unscheduled task.
Switch to standard ID ordering by default.
Test Plan:
- Instances console now finds unscheduled triggers.
- Verified that all existing queries specify an explicit order.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11436
Summary: Ref T6881. When stuff with triggers is destroyed, it should destroy the triggers.
Test Plan: Will test in Instances.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11435
Summary: Ref T6881. Add a standard "just queue a task" trigger action; I expect almost all application code to use this.
Test Plan: Will test in Instances.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11429
Summary: Ref T6881. I just want to show trigger info in the instance management console.
Test Plan: Will test in Instances.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11428
Summary: Ref T6881. Before implementing subscriptions, I'm going to vet triggers by using them to do backups. Each instance will get a daily trigger for backups, and that should give us a smaller-scale test to catch issues and limitations, with more opportunities for something to go wrong since it fires more often.
Test Plan: Added unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11427
Summary: Ref T5952, T3404. This lays the basic plumbing for how this will work, all the way to deploying on Maniphest. Aside from what is mentioned on T5952, I think page(s) on editing application emails could use a little more helpful text about what's going on, similar to how the config page that's getting deprecated works.
Test Plan: ran migration and noted my create email address migrated successfully. used bin/mail to make a task. added another email and used bin/mail to make a task. deleted an email. edited an email. invoked various error states and they all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404, T5952
Differential Revision: https://secure.phabricator.com/D11418
Summary: Fixes T6963. Long term will likely make this more like other document views, but not worth the time right now since this is only location.
Test Plan: Review Phriction document at desktop and mobile breakpoints. Click menu and see menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6963
Differential Revision: https://secure.phabricator.com/D11420
Summary:
Taking a pass at revamping the edit pages in Projects. Specifically:
- Remove EditMainController
- Move actions from EditMain to Profile
- Move properties from EditMain to Profile
- Move timeline from EditMain to Profile
- Move Open Tasks from Profile to sidenavicon
- Add custom icons and colors to timeline
Feel free to bang on this a bit and give feedback, feels generally correct to me.
Test Plan: Edit everything I could on various projects. Check links, timelines, actions.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11421
Summary:
Ref T6881. Hopefully, this is the hard part.
This adds a new daemon (the "trigger" daemon) which processes triggers, schedules them, and then executes them at the scheduled time. The design is a little complicated, but has these goals:
- High resistance to race conditions: only the application writes to the trigger table; only the daemon writes to the event table. We won't lose events if someone saves a meeting at the same time as we're sending a reminder out for it.
- Execution guarantees: scheduled events are guaranteed to execute exactly once.
- Support for arbitrarily large queues: the daemon will make progress even if there are millions of triggers in queue. The cost to update the queue is proportional to the number of changes in it; the cost to process the queue is proportional to the number of events to execute.
- Relatively good observability: you can monitor the state of the trigger queue reasonably well from the web UI.
- Modular Infrastructure: this is a very low-level construct that Calendar, Phortune, etc., should be able to build on top of.
It doesn't have this stuff yet:
- Not very robust to bad actions: a misbehaving trigger can stop the queue fairly easily. This is OK for now since we aren't planning to make it part of any other applications for a while. We do still get execute-exaclty-once, but it might not happen for a long time (until someone goes and fixes the queue), when we could theoretically continue executing other events.
- Doesn't start automatically: normal users don't need to run this thing yet so I'm not starting it by default.
- Not super well tested: I've vetted the basics but haven't run real workloads through this yet.
- No sophisticated tooling: I added some basic stuff but it's missing some pieces we'll have to build sooner or later, e.g. `bin/trigger cancel` or whatever.
- Intentionally not realtime: This design puts execution guarantees far above realtime concerns, and will not give you precise event execution at 1-second resolution. I think this is the correct goal to pursue architecturally, and certainly correct for subscriptions and meeting reminders. Events which execute after they have become irrelevant can simply decline to do anything (like a meeting reminder which executes after the meeting is over).
In general, the expectation for applications is:
- When creating an object (like a calendar event) that needs to trigger a scheduled action, write a trigger (and save the PHID if you plan to update it later).
- The daemon will process the event and schedule the action efficiently, in a race-free way.
- If you want to move the action, update the trigger and the daemon will take care of it.
- Your action will eventually dump a task into the task queue, and the task daemons will actually perform it.
Test Plan:
Using a test script like this:
```
<?php
require_once 'scripts/__init_script__.php';
$trigger = id(new PhabricatorWorkerTrigger())
->setAction(
new PhabricatorLogTriggerAction(
array(
'message' => 'test',
)))
->setClock(
new PhabricatorMetronomicTriggerClock(
array(
'period' => 33,
)))
->save();
var_dump($trigger);
```
...I queued triggers and ran the daemon:
- Verified triggers fire;
- verified triggers reschedule;
- verified trigger events show up in the web UI;
- tried different periods;
- added some triggers while the daemon was running;
- examined `phd debug` output for anything suspicious.
It seems to work in trivial use case, at least.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11419
Summary:
Ref T6881. This will probably make more sense in a couple of diffs, but this is a class that implements scheduling/recurrence rules. Two rules are provided:
- Trigger an event at a specific time (e.g., a meeting reminder notification).
- Trigger an event on the Nth day of every month (e.g., a subscription bill).
At some point, we'll presumably add a rule for T2896 (maybe using the "RRULE" spec) so you can do stuff like "the second to last thursday of every month", etc., but we don't need that for now.
(The "Nth day of every month, or move it back if no such day exists" rule doesn't seem to be expressible with the "RRULE" format, so implementing that wouldn't give us a superset of this. I think this rule is correct and desirable for this purpose, though.)
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11403
Summary:
This is unusual, but if `getWorkerInstance()` throws we end up with an undefined `$worker` when recovering from the exception.
Instead, handle this case slightly more gracefully.
The easiest way to hit this is to schedule a task for a worker that doesn't exist (or remove an existing worker, which is what I did to hit it).
Test Plan: Saw a more graceful error recovery; ran some normal successful tasks out of the queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11413
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11415
Summary: Ref T6822. This method needs to be `public` because it is called from `PhabricatorApplicationSearchController::buildApplicationMenu()`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11416
Summary: Ref T6822. This method is only called from `PhutilDaemon::execute()` and can be made `protected`.
Test Plan: See D11404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11405
Summary: Ref T6822. This method is only called from within the `PhabricatorWorker::executeTask()` and `PhabricatorWorker::scheduleTask()` methods.
Test Plan: `grep`ped for `->doWork`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11406
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from `LiskDAO::update()`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11409
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `LiskDAO::establishConnection()`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11412
Summary: Ref T6944. This was not quite implemented correctly in D11387.
Test Plan: Saw no more exceptions about being unable to create `/var/log`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: maxhodak, Korvin, epriestley
Maniphest Tasks: T6944
Differential Revision: https://secure.phabricator.com/D11397
Summary: ...also adds policies on who can view and who can edit an action. Fixes T6949.
Test Plan: viewed a secret through the new UI and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6949
Differential Revision: https://secure.phabricator.com/D11401
Summary: Ref T6962. Mainly accomplished by re-factoring the base editor `buildMailBody` function and then using it differently in the `DifferentialTransactionEditor`.
Test Plan: commented on a revision leaving inline feedback. inspected via bin/mail and it looked good! also made a maniphest comment and checked that email, which still looked good.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6962
Differential Revision: https://secure.phabricator.com/D11402
Summary: Fixes T6959.
Test Plan: When I was ready to test the feature, the "Daemon & Web config" error already showed up, from having added phd.variant-config. I went meta and changed the value of phd.variant-config to have phd.variant-config. The config error disappeared. I then changed the conpherence setting about conpherence email prefix and the error showed up again. Removing the conpherence config setting made the error disappear once more.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6959
Differential Revision: https://secure.phabricator.com/D11399
Summary:
Ref T6881. This is part 1 of my 35-step plan to support subscriptions that bill monthly.
Expanding the capabilities of counters will let me use them to create a logical clock on time-based event updates, build a daemon on top of that, and eventually get time-based triggers.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11395
Summary:
For block-level elements that have a margin-top or margin-bottom set
(generally to 12px), also reset the appropriate margin to 0 when
they're a first-child or last-child of their parents.
The change doesn't affect nested lists, their selector is more specific.
Test Plan:
Look at some comments or wiki documents that end with different
block elements, verify that the margins are pretty.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: Korvin, epriestley
Projects: #remarkup
Maniphest Tasks: T6968
Differential Revision: https://secure.phabricator.com/D11382
Summary: Fixes T6842. Allow the daemons to always be terminated, even if MySQL is down. I was hoping to be able to optionally enable this behavior with the `--force` flag, but this seems messy.
Test Plan:
```lang=bash
> ./bin/phd start
Freeing active task leases...
Freed 1 task lease(s).
Preparing to launch daemons.
NOTE: Logs will appear in '/var/tmp/phd/log/daemons.log'.
Starting daemons as phd
Launching daemon "PhabricatorRepositoryPullLocalDaemon".
Starting daemons as phd
Launching daemon "PhabricatorGarbageCollectorDaemon".
Starting daemons as phd
Launching daemon "PhabricatorTaskmasterDaemon".
Done.
> service mysql stop
mysql stop/waiting
> ./bin/phd stop
Interrupting daemon 'PhabricatorRepositoryPullLocalDaemon' (4263)...
Interrupting daemon 'PhabricatorGarbageCollectorDaemon' (4271)...
Interrupting daemon 'PhabricatorTaskmasterDaemon' (4287)...
Daemon 4263 exited.
Daemon 4271 exited.
Daemon 4287 exited.
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6842
Differential Revision: https://secure.phabricator.com/D11385
Summary: Fixes T6944. Attempt to automatically create the log directory for the Aphlict server. If the directory can't be created, throw a helpful exception.
Test Plan:
# Set `notification.log` to `/var/log/aphlict/aphlict.log`.
# Ran `./bin/aphlict debug` and saw an exception (because the user doesn't have permissions to create the `/var/log/aphlict` directory).
# Ran `sudo chmod 777 /var/log`.
# Ran `./bin/aphlict debug` and saw the `/var/log/aphlict` directory created.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6944
Differential Revision: https://secure.phabricator.com/D11387
Summary: I assume we've shown this long enough, plus with redesign it's a good time to remove.
Test Plan: reload page, no link
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11394
Summary: We still seem to reach for this, though may be time to remove Wiki?
Test Plan: view link
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11393
Summary: In Maniphest, we provide an additional caption shortcut if you can create projects, which has no use if you cant. Fixes T6969
Test Plan: Check page with and without a user's capability.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6969
Differential Revision: https://secure.phabricator.com/D11390
Summary: Ref T6971. This fixes the error the user reported. Not sure what's up with the root cause of their issue.
Test Plan: Went to `/auth/config/new/asdfqwer/` and got a 404 instead of an exception.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6971
Differential Revision: https://secure.phabricator.com/D11388
Summary: Fixes T6955.
Test Plan: made an oauth app. made a test authorization. ran bin/remove destroy <phid of oauth client> and there were no errors. verified oauth app and test authorization were both gone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6955
Differential Revision: https://secure.phabricator.com/D11378
Summary: Fixes T6957. If / when a policy object is destroyed, access to an object that uses that policy object is denied.
Test Plan: looked around in the code to fail confident enough to write the summary above
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6957
Differential Revision: https://secure.phabricator.com/D11380
