Summary: Ref T7094. We basically need to make sure folks can see repositories before making owners packages about code within. This cleans up things a little bit by moving a bunch of logic out of the storage class and into an editor class.
Test Plan: made a package and it worked! deleted a package and it worked! discovered buggy behavior in more complicated edits and filed T7127; note this bug exists before and after this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11652
Summary:
Ref T7123. Two general issues:
For proxied repositories, we currently throw a ConduitClientException, vs ConduitException for local repositories. This is inconsistent and we should fix it, but I also want to examine the use of try-the-call-and-throw at these sites since it may be something we can update. In particular, trying a call that we know will always fail is now more expensive (in proxied repositories) than it used to be.
Here, we try-and-throw for merges, but they're //never// supported in Subversion. Just don't bother trying.
Test Plan: Browsed a SVN repository with proxying set up, got a clean commit page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7123
Differential Revision: https://secure.phabricator.com/D11646
Summary:
Fixes T7122. The way this query works is a little surprising:
- If executed as `withRepositoryIDs(...)`, it assumes you are passing one //or more// repository IDs, so it will never resolve ambiguous identifiers (e.g., "123" instead of "rSVN123").
- If executed as `withRepository(...)`, it knows you are passing exactly one repository and will use that to imply context and resolve these identifiers correctly.
This isn't very obvious from the API, but I'm not sure how to make it more clear.
(Making `withRepositoryIDs()` do the `withRepository(...)` thing if only one ID was passed in would mean its behavior varied if you passed 1 vs 2 repository IDs, which seems worse / morse surprising.)
Test Plan: Various subversion UIs no longer fail to look up commits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mormegil, epriestley
Maniphest Tasks: T7122
Differential Revision: https://secure.phabricator.com/D11645
Summary: Minor, adds border, reduces greys, etc.
Test Plan:
View a number of config issues, see new colors.
{F282035}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11650
Summary: Just makes the UI cleaner on full width or dialog forms (and mobile)
Test Plan:
run into a bunch of errors, test mobile breakpoints.
{F281585}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11638
Summary:
This is a pain to test, but we do a lot of needless "X committed thing (authored by X)" right now.
I think that's because we compare two handle links here, and they're never the same, even if they're both links to the same object.
Instead, compare the author and committer more carefully.
Test Plan: Will do it live.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11635
Summary: Ref T7094. I am not sure when this text is legitimately exposed to users - they should be getting an error about not being able to see the object before they get an error about not being able to see a given transaction... That said, I think this text is logically correct at least.
Test Plan: read the text
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11632
Summary: should just be "withIDs" Ref T7094
Test Plan: submitting this very diff!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11633
Summary: This got updated recently but isn't quite correct.
Test Plan: Called `arcanist.projectinfo` using the name of a proejct with a repository association.
Reviewers: btrahan
NOTE: Cowboy committing this since it breaks `arc diff`.
Summary: We probably can't land this yet, since `arc tasks` still uses `maniphest.find` and `arc close` still uses `differential.getrevision`. We should clean those up and wait at least 30 days before committing this (maybe).
Test Plan: Saw setup issues for `maniphest.find` and `differential.getrevision` calls.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, joshuaspence, FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D6333
Summary: Ref T7094. We already had and were mostly using "needProfileImage" on the people query class. Only real trick in this diff is deleting a conduit end point that has been marked deprecated for the better part of 3 years.
Test Plan: clicked around the people action and profiles and calendars loaded nicely.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11630
Summary: Not too shabby - just convert some raw queries to the policy queries. Ref T7094.
Test Plan: NA 'cuz releeph
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11591
Summary: Ref T7094. This one is really straight-forward since $this->actor is always populated and the right thing to do here.
Test Plan: used the ole thinking noodle since testing email w/ attachments is really hard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11589
Summary: Ref T7094. This loadRepository() method bypassed policy unnecessarily. kill it.
Test Plan: basically un-tested since arcanist projects are deprecated and the main callsites were in releeph. conduit end point still works though!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11586
Summary: Revamps Profile to be like Projects, a mini portal and side nav with icons.
Test Plan: Viewed my own profile, as well as others. Test seeing my commits, tasks, diffs, and upcoming events. Checked mobile navigation.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11547
Summary: Use `PhutilXHPASTBinary` methods instead of `xhpast_parse` functions. Depends on D11517.
Test Plan: N/A, this is a direct swap.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11612
Summary: This sets an icon for each config, makes it easier to scan.
Test Plan:
Reload Config page, see all new icons
{F281089}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11619
Summary: Clean up the error view styling.
Test Plan:
Tested as many as I could find, built additional tests in UIExamples
{F280452}
{F280453}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11605
Summary: Debugging crumbs in repository editing, and it seems there are stray divs that aren't used from extending AphrontTagView. I don't see any specific reason this needs to be from AphrontTagView, so changing it. Of course I'm not sure this is correct, so feel free to reject if I'm missing some obvious or non-obvious reasons.
Test Plan: Review editing a repositor, don't see extra div.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11608
Summary: Ref T6881. This won't do much of interest on third party installs yet, but it's stable and we don't need to hold it back any longer.
Test Plan: Ran `phd start`, saw the trigger daemon start up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11603
Summary: Ref T6881. If we can't automatically bill an invoice, send the account owners a mail explaining why and asking them to pay it.
Test Plan: {F279596}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11602
Summary:
Ref T6881.
- Fix dead links.
- Let implementations provide more information.
- Provide more information to implementations.
Test Plan: Links work, invoices show billing periods, fewer "Subscription 6" crumbs, all is well in the world.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11601
Summary:
Ref T6881.
- Allow users to set a default payment method for a subscription, which we'll try to autobill (not all payment methods are autobillable, so we can't require this in the general case, and a charge might fail anyway).
- If a subscription has an autopay method, try to automatically bill it.
- Otherwise, we'll send them an email like "hey here's a bill, it couldn't autopay for some reasons, go pay it and fix those if you want".
- (That email doesn't exist yet but there's a comment about it.)
- Also some UI cleanup.
Test Plan:
- Used `bin/phortune invoice` to autobill myself some fake test money.
{F279416}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11596
Summary: Self-explanatory. Also made a few methods `final`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11598
Summary: Ref T7094. The class DiffusionRequest has other public methods which use getUser() in an unguarded way. Code inspection of the call sites for loadCommit() also leads me to believe the $user is properly set.
Test Plan: clicked around diffusion a bunch and everything seemed to work okay. (happy to test any particular esoteric endpoints that come to mind)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11585
Summary:
Ref T6881. This is basically just some UX.
Right now, if we invoice you, you can //technically// pay it but since we don't tell you about it and don't show it in the UI you'd have to guess the ID by manipulating the URI. We should probably be at least a little more aggressive about billing.
In the common case when we generate a cart/order, we don't show it to the user or merchant in Phortune until the user takes a payment action (basically, Phortune doesn't recognize the cart until you actually check out with it). In the current use case in Fund (and other reasonable use cases) an un-acted-upon cart hasn't been ordered yet, and is just a place for the application to store state as it hands off the workflow to Phortune.
Even if we had a real "Shop for physical goods" app, I think the same rule would apply -- the application itself would probably track and show your current cart, but it wouldn't make sense to put it into your order history in Phortune until you actually buy it.
Since invoices from subscriptions are essentially identical to not-yet-ordered-carts, that mean they also did not show up in the UI (although I think this is also desirable).
This change carves out a place for them:
- Add an "invoices" section with unpaid invoices.
- The UI shows that you have unpaid invoices.
- Invoices have a slightly different rendering, inclduing an alluring "Pay Now" button.
Some considerations:
- One thing I'm vaguely thinking about is the possibilty that users may be able to invoice one another directly, eventually. For example, we might invoice a contracting client.
- Considering this, I thought about making these carts have a special status like `STATUS_DUE`, which replaces `STATUS_READY`, or a flag like `isInvoice`.
- However, this approach was pretty involved and made the //billing// logic more complicated, so I backed off. The ultimate approach here puts more of the complexity into the display logic, which feels better to me.
- We might need an `isInvoice` flag eventually, but `subscriptionPHID` is a reasonable stand-in for now.
- The OrderTable serving double duty for rendering subscriptions feels a little muddy, but I think splitting it into two highly-redundant classes would be worse.
Test Plan:
{F279348}
{F279349}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11593
Summary: The method is actually named `DivinerAtomRef::newFromDictionary`.
Test Plan: `./bin/diviner generate --publisher DivinerStaticPublisher` worked a bit better.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11590
Summary: Allow the `DivinerPublisher` subclass to be specified via `./bin/divner generate --publisher ...`. In particular, this allows use of the (mostly broken) `DivinerStaticPublisher`.
Test Plan: Ran `./bin/diviner generate --publisher DivinerStaticPublisher`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11588
Summary: Minor tidying and modernizing a few things.
Test Plan: Ran `./bin/diviner atomize` and `./bin/diviner generate`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11587
Summary: Ref T7094.
Test Plan: couldn't really test this - how does one get symbols going nowadays given they are acanist project based?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11584
Summary:
Ref T6881. This generates a product, purchase and invoice for users, and there's sort of some UI for them. Stuff it doesn't do yet:
- Try to autobill when we have a CC;
- actually tell the user they should pay it;
- ask the application for anything like "how much should we charge", or tell the application anything like "the user paid".
However, these work:
- You can //technically// pay the invoices.
- You can see the invoices you paid in the past.
Test Plan: Used `bin/phriction invoice` to double-bill myself over and over again. Paid one of the invoices.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11580
Summary: Ref T7094. Could just delete this end point too I guess? Needed to add "withCommitPHIDs" to the differentialrevisionquery to get this done.
Test Plan: used diffusion.getcommits from conduit console and got a sensible result for a query for two commits, one with a diff and one without.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11581
Summary:
Ref T6881. This adds the worker, and a script to make it easier to test. It doesn't actually invoice anything.
I'm intentionally allowing the script to double-bill since it makes testing way easier (by letting you bill the same period over and over again), and provides a tool for recovery if billing screws up.
(This diff isn't very interesting, just trying to avoid a 5K-line diff at the end.)
Test Plan: Used `bin/phortune invoice ...` to get the worker to print out some date ranges which it would theoretically invoice.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11577
Summary:
Ref T6881.
- Add a subscription detail page.
Minor cosmetics:
- Fix glyph, from "X" (old "X marks the spot" icon) to "diamond" (new gem icon).
- Name the initial account "Default Account" instead of "Personal Account", since this seems more general.
Test Plan:
{F278623}
And I got two full days to test that Jan 30/31 -> Feb 28 billing logic!
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11576
Summary:
Ref T6881. This still doesn't "work" in any reasonable sense of the word, but gets us a bit further.
I'll build out the Phortune UI a little bit next, then look at implementing the Worker to do actual billing.
Test Plan:
- Allocated an instance and saw a Subscription generate properly.
- Saw subscription show up in the Phortune UI, albeit in a very limited way.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11575
Summary:
Ref T7094. We should do a policy query on the files IMO because there exists a scenario where the file gets locked down directly. This requires being a bit more disciplined about setting user, which in turn requires deciding whether or not to show edit / reply links as a separate piece of logic, not conditional on user presence.
This is not the best code but I don't think it gets worse with this and is just some other nuance in any larger cleanup we take on someday.
Test Plan: looked at a revision and noted inline comments rendered correctly with reply / edit actions. looked at a diff standalone and noted no reply / edit actions as expected. looked at a "details" link on a transaction and it rendered correctly. looked at a diff in phriction of page edits and it looked good. grepped around and verified the remaining callsite in diffusion already has the setUser call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11579
Summary: Fixes T1476. The body of the email should be just the output of some diff command.
Test Plan:
git diff master > text.txt; ./bin/mail receive-test --to <configured-diff-create-address> < text.txt; a diff was successfully created...! email generated had a working link to the diff.
./bin/mail receive-test --to <configured-diff-create-address> < README.md; a diff was not created as expected...! email generated had a sensical error message, telling me that the mail body should have been generated via a diff command
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: johnny-bit, Korvin, epriestley
Maniphest Tasks: T1476
Differential Revision: https://secure.phabricator.com/D11574
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
Summary:
Fixes T6548.
- This workflow doesn't work under reasonable configurations and isn't trivial to fix (see T6548).
- We don't need it; this just makes things a little bit faster if you have to migrate everything (e.g., immediately after T1191) and the installs we know about have generally upgraded by now.
- This keeps kicking PKCS8 keys out of cache which is a pain.
Test Plan: Ran `bin/storage adjust` without it doing an implicit cache purge.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6548
Differential Revision: https://secure.phabricator.com/D11377
Summary: Fixes T6964, makes action links float instead of absolutely positioned.
Test Plan: Tested UIExamples, actions in single line headers, multi line headers, headers with images, workboard headers. Test desktop, mobile, and tablet breakpoints. Long titles wrap as expected as button list grows.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6964
Differential Revision: https://secure.phabricator.com/D11379
Summary: We could still miss this if the policy had never been customized and we returned early after one of the other checks.
Test Plan:
Works great on instances now.
{F267067}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11374
Summary: Third time lucky... the filename should match the class name now.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11362
Summary: Ref T6822. There are a bunch of places where we call `$something->generatePHID(...)` externally (outside of the class). Therefore, these methods need to be `public`.
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/D11363
Summary: Fast commit. Also forgot to make the config override the existing policy. I *think* this is the right spot and we're good? Ref T6947.
Test Plan: viewed the application settings page for people application and saw the correct overrode setting.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6947
Differential Revision: https://secure.phabricator.com/D11373
Summary: Ref T5833. This doesn't do anything yet, but will allow new instances to automaticaly bind to an open database without anything too hacky.
Test Plan:
Created a service of this type.
{F267059}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D11372
Summary: Fixes T6947
Test Plan:
locked people.create.user and noted the UI only showed a link to the existing policy with no way to edit it.
tried to set the config to all the various bad things and saw helpful error messages telling me what I did wrong.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6947
Differential Revision: https://secure.phabricator.com/D11358
Summary: Not sure this is obvious enough, but maybe future apps will use as well?
Test Plan: test a project with and without a workboard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11371
Summary: Ref T6947.
Test Plan: made the setting say only admin user a and noted admin user b lost access
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4137, T6947
Differential Revision: https://secure.phabricator.com/D11357
Summary: Reduces visual duplication in a few places.
Test Plan: Review pages in sandbox, see image removed.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11354
Summary: Ref T6947.
Test Plan: toggled setting in application settings and changes stuck. set policy to admin user a only and could not add a provider as a admin user b.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6947
Differential Revision: https://secure.phabricator.com/D11356
Summary: Fixes T6895, When viewing comment edit history, user should not see a dropdown for each comment edit transaction.
Test Plan: Edit task comment, view comment edit history, comment transactions should not provide a dropdown with action items.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6895
Differential Revision: https://secure.phabricator.com/D11355
Summary: Fixes T6917, swallow exception when saving blocking tasks with no changes
Test Plan: Open task, "Edit Blocking Tasks", save without changing, dialog should close with no exception
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6917
Differential Revision: https://secure.phabricator.com/D11353
Summary:
Fixes T5352. This is very useful for finding things that should be easy to do ("not blocked") as well as things that are important to do ("blocking"). I have wanted to check out the latter case in our installation, though no promises on what I would end up actually doing from that search result list. =D
I also think supporting something like T6638 is reasonable but the UI seems trickier to me; its some sort of task tokenizer, which I don't think we've done before?
Test Plan: toggled various search options and got reasonable results. When i clicked conflicting things like "blocking" and "not blocking" verified it was like I had not clicked anything at all.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5352
Differential Revision: https://secure.phabricator.com/D11306
Summary: Adds more user friendly copy to the result list
Test Plan: Test on a project with and without tasks.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11352
Summary: In my use case, I have `notification.client-uri` set to `https://phabricator.example.com/ws/` (which routes to `nginx`) but I need `aphlict` to listen to port `22280`.
Test Plan: Tested in our install.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11337
Summary:
A refresh of Projects including a new navigations UI.
- New Navigation UI.
- Auto switch default page if Workboard has been initialized
- Move Feed to it's own page
- Increase 'tasks' on Project Home to 50 over 10
- Fix various display bugs on Workboards
- Remove 'crumbs' from Project portal (unneeded).
Test Plan:
- clicked a link for a project with no workboard and saw the profile
- clicked a link for a project with a workboard and saw the workboard
- navigated around the various edit pages, inspecting links and making sure things linked back to the new profile uri
{F266460}
{F266461}
{F266462}
{F266463}
{F266464}
Reviewers: epriestley, btrahan
Reviewed By: epriestley, btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11272
Summary: Ref D11340, I missed the comments being to excited to land.
Test Plan: Shrink window to mobile view, click on action menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11347
Summary:
Yeahhhhhhhh....
- Open a "stream", not a "steam".
- Make error easier for users to understand.
- Write to the log in debug mode so the issue is more apparent.
Test Plan:
- Started server with bad permissions, got usable error message.
- Started server with good permissions, got logfile.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11339
Summary: Ref T5752, moves mobile action menus to the object box instead of crumbs.
Test Plan: View action menus at tablet, desktop, and mobile break points. Verify clicking buttons works as expected opening menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5752
Differential Revision: https://secure.phabricator.com/D11340
Summary: Fixes T6937. We weren't passing required parameters.
Test Plan: Followed repro steps in task.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6937
Differential Revision: https://secure.phabricator.com/D11346
Summary:
Fixes T6932. Fixes some issues from D11303.
- When claiming a task, if it was previously unassigned, we would try to CC `null`.
- When claiming a task, if the current owner was already CC'd, the viewer would incorrectly be warned about all subscribers being CC'd.
Test Plan:
- Claimed an unclaimed task.
- Claimed a task with owner CC'd.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T6932
Differential Revision: https://secure.phabricator.com/D11336
Summary: Fixes T6922. We should allow the commit pipeline to continue on certain types of exceptions, including `PhabricatorApplicationTransactionNoEffectException`.
Test Plan:
**Before**
```lang=bash
> ./bin/repository reparse --herald rP2660b944bed4e4dde3e66303656b1d96d8b03e9b
[2015-01-10 09:38:06] EXCEPTION: (PhabricatorApplicationTransactionNoEffectException) Transactions have no effect:
- Edges already exist; transaction has no effect. at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1635]
#0 PhabricatorApplicationTransactionEditor::filterTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:649]
#1 PhabricatorApplicationTransactionEditor::applyTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php:91]
#2 PhabricatorRepositoryCommitHeraldWorker::parseCommit(PhabricatorRepository, PhabricatorRepositoryCommit) called at [<phabricator>/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php:44]
#3 PhabricatorRepositoryCommitParserWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
#4 PhabricatorWorker::executeTask() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php:297]
#5 PhabricatorRepositoryManagementReparseWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:396]
#6 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:292]
#7 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/repository/manage_repositories.php:22]
```
**After**
```lang=bash
> ./bin/repository reparse --herald rP2660b944bed4e4dde3e66303656b1d96d8b03e9b
Done.
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6922
Differential Revision: https://secure.phabricator.com/D11304
Summary: Fixes T6732. Fix is to stop trying to catch the error in the controller and let the editor do its job.
Test Plan: tried to add an existing subscriber and got an error message about how that wouldn't do anything
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6732
Differential Revision: https://secure.phabricator.com/D11303
Summary:
Fixes T4656. Helps users with this naming convention, which is probably not super duper rare.
Users will need to make an edit to a project -or- run bin/search index "#project-tag" to make this actually work.
Test Plan: made a project "[T4656test]". Typed "t4" and project showed up!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4656
Differential Revision: https://secure.phabricator.com/D11302
Summary: Fixes T6923. Turns out we can't use the editor since we don't have a user with a phid (just some omnipotent guy).
Test Plan: ./bin/config set --database syntax.filemap '{}'; ./bin/config delete --database syntax.filemap
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6923
Differential Revision: https://secure.phabricator.com/D11301
Summary: Fixes T5646. Makes diffusion a much better user experience. Users now see a 404 exception page when they have a bad URI. Previously, they saw a developer-facing raw exception.
Test Plan: played around in diffusion a bunch. most of these changes were fairly mechanical at the end of the day.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5646
Differential Revision: https://secure.phabricator.com/D11299
Summary: Fixes T6471. This makes adding projects to tasks have better workflow towards boards; without it, you have to click project -> board -> do stuff on board as opposed to column -> do stuff on board.
Test Plan: added and removed projets. saw column listed parenthetcally when expected
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6471
Differential Revision: https://secure.phabricator.com/D11260
Summary: This is the hardening work mentioned in T887#86529. Also take a documentation pass for accuracy about these changes and formatting. Ref T4593.
Test Plan: unit tests...! generated diviner docs and oauthserver doc looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4593
Differential Revision: https://secure.phabricator.com/D11298
Summary: Fixes T6594, Logged out users should be able to "View Raw" comments in public objects.
Test Plan: Log out, open maniphest task with comments, open dropdown associated with comment, click "View Raw", should be able to see raw comment.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6594
Differential Revision: https://secure.phabricator.com/D11295
Summary:
Ref T6870. Since it does not make sense to redirect the user to the login form after they log in, we try not to set the login form as the `next` cookie.
However, the current check is hard-coded to `/auth/start/`, and the form can also be served at `/login/`. This has no real effect on normal users, but did make debugging T6870 confusing.
Instead of using a hard-coded path check, test if the controller was delegated to. If it was, store the URI. If it's handling the request without delegation, don't.
Test Plan:
- Visited login form at `/login/` and `/auth/start/`, saw it not set a next URI.
- Visited login form at `/settings/` (while logged out), saw it set a next URI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lpriestley
Maniphest Tasks: T6870
Differential Revision: https://secure.phabricator.com/D11292
Summary: This is a little rough and should be considered an "advanced" option. Having said that, this works well in my install and I imagine that other installs will find this beneficial.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11293
Summary: So meta it hurts. Fixes T887.
Test Plan: created a second instance of phabricator locally. made an account on oauth server phabricator. set up my normal dev phabricator to use this new oauth phabricator. noted the form worked. created an account via the oauth method and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T887
Differential Revision: https://secure.phabricator.com/D11287
Summary: If a cookie prefix is set (as on the Phacility cluster), we end up double-namespacing cookies when trying to remove them. This can make logging out produce a cookie error.
Test Plan: Logged out locally with cookie prefix, got normal logout workflow.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11282
Summary: Fixes T6883, Legalpad action button on edit document page should say "Save Document" instead of "Edit Document"
Test Plan: Open Legalpad, open existing document, blue action button should say "Save Document"
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6883
Differential Revision: https://secure.phabricator.com/D11291
Summary: If you are running the Aphlict server behind a reverse proxy (such as `nginx`) then there's no need to bind to `0.0.0.0`. Add a `--client-host` flag to `aphlict_server.js` to allow binding to a different hostname. Also changed the other flags for consistency and clarity.
Test Plan: Started, stopped and debug the Aphlict server.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11288
Summary: When viewing a task with a mailing list CC where the mailing list is public, logged out user should see the name of the mailing list
Test Plan: Create public mailing list, CC mailing list on task, logout and view task, mailing list name should still be visible on the task
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11290
Summary: Fixes T6870, logging in from a public object should land on that object.
Test Plan: Navigate to a maniphest task in a logged out state, login, landing page should be maniphest task.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6870
Differential Revision: https://secure.phabricator.com/D11289
Summary: Fixes T6910. This advice is bad, doesn't work, and was based on me havng an outdated or incorrect understanding of Node and npm.
Test Plan: Read documentation.
Reviewers: richardvanvelzen, btrahan, chad, joshuaspence
Reviewed By: chad, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T6910
Differential Revision: https://secure.phabricator.com/D11285
Summary: This was omitted in D11143.
Test Plan: I don't always test, but when I do... I do it in production.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11284
Summary:
Fixes T6559. No more flash, use Websockets. This is less aggressive than the earlier version, and retains more server logic.
- Support "wss".
- Make the client work.
- Remove "notification.user" entirely.
- Seems ok?
Test Plan:
In Safari, Firefox and Chrome, saw the browsers connect. Made a bunch of comments/updates and saw notifications.
Notable holes in the test plan:
- Haven't tested "wss" yet. I'll do this on secure.
- Notifications are //too fast// now, locally. I get them after I hit submit but before the page reloads.
- There are probably some other rough edges, this is a fairly big patch.
Reviewers: joshuaspence, btrahan
Reviewed By: joshuaspence, btrahan
Subscribers: fabe, btrahan, epriestley
Maniphest Tasks: T6713, T6559
Differential Revision: https://secure.phabricator.com/D11143
Summary:
Ref T6559. See discussion in D11143. At least locally, WebSockets are too fast and create immediate local notifications on page submit.
To mitigate this, don't notify about your own actions.
This isn't perfect (we get the other-copies-of-the-window-open-in-other-tabs case wrong) but I think the case we get wrong is rare / not very important.
Test Plan: Submitted stuff, saw other users get notifications but not me.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T6559
Differential Revision: https://secure.phabricator.com/D11275
Summary: Derped this up in D11234.
Test Plan: Ran `bin/search index --all`.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11273
Summary: Fixes T6863. Seems like this belongs there?
Test Plan: loaded up an API in conduit console and saw the new error text
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6863
Differential Revision: https://secure.phabricator.com/D11261
Summary: Fixes T6179. This makes the interaction where users remove a task from a workboard much more pleasant.
Test Plan: Loaded up workboard for "A Project". Edited tasks and if / when I removed "A Project" they disappeared on save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6179
Differential Revision: https://secure.phabricator.com/D11259
Summary: Ref T6822.
Test Plan: Visual inspection. This method is only called from within the `AphrontBarView` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11240
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within `PhabricatorController` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11241
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplicationSearchEngine` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11242
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplication` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11243
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplicationTransactionEditor` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11244
Summary: Ref T6822.
Test Plan: Visual inspection. This method is only called from within the `PhabricatorTestCase` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11245
Summary: Ref T6822.
Test Plan: Visual inspection. This method is only called from within `PhabricatorOAuthAuthProvider` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11246
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorConfigStorageSchema` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11247
Summary: Ref T6822.
Test Plan: Visual inspection. This method is only called from within the `PhabricatorRepositoryCommitParserWorker` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11248
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `AphrontFormControl` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11249
Summary: Fixes T6555, The following should 404: /maniphest/task/create/?parent=asdf, /maniphest/task/create/?parent=0, /maniphest/task/create/?parent=999999 (where T999999 does not exist)
Test Plan: Navigate to /maniphest/task/create/?parent=asdf or /maniphest/task/create/?parent=0 or /maniphest/task/create/?parent=999999 (where T999999 does not exist). See 404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6555
Differential Revision: https://secure.phabricator.com/D11258
Summary:
Fixes T5966. Accomplishes a few things
- see title
- adds a force-autoclose flag and the plumbing for it
- removes references to some HarborMaster thing that used to key off commits and seems long dead, but forgotten :/
Test Plan:
ran a few commands. These first three had great success:
`./repository reparse --all FIRSTREPO --message --change --herald --owners`
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date yesterday`
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date yesterday --force-autoclose`
...and these next two showed me some errors as expected:
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date garbagedata`
`./repository reparse --all GARBAGEREPO --message --change --herald --owners`
Also, made a diff in a repository with autoclose disabled and commited the diff. Later, reparse the diff with force-autoclose. Verified the diff closed and that the reason "why" had the proper message text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T5966
Differential Revision: https://secure.phabricator.com/D10492
Summary: Quick pass at cleaning up language, icons, colors for 'Archive' and 'Activate' on various objects.
Test Plan:
Tested archiving and activating each object changed.
{F262694}
{F262697}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11256
Summary: We technically don't use K&R. These documents and the rest of the codebase are full of examples of the correct style, which should be unambiguous to a reasonable reader.
Test Plan: reading
Reviewers: staticshock, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11255
Summary:
Ref T3165. Builds a dedicated index for Conpherence to avoid scale/policy filtering concerns.
- This is pretty one-off but I think it's generally OK.
- There's no UI for it.
- `ConpherenceFulltextQuery` is very low-level. You would need to do another query on the PHIDs it returns to actually show anything to the user.
- The `previousTransactionPHID` is so you can load chat context efficiently. Specifically, if you want to show results like this:
> previous line of context
> **line of chat that matches the query**
> next line of context
...you can read the previous lines out of `previousTransactionPHID` directly, and the next lines by issuing one query with `WHERE previousTransactionPHID IN (...)`.
I'm not 100% sure this is useful, but it seemed like a reasonable thing to provide, since there's no way to query this efficiently otherwise and I figure a lot of chat might make way more sense with a couple of lines of context.
Test Plan:
- Indexed a thread manually (whole thing indexed).
- Indexed a thread by updating it (just the new comment indexed).
- Wrote a hacky test script and got reasonable-looking query results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D11234
Summary: Fixes T6880. If matching commits have no visible/loadable repository, we shouldn't keep going forward in the loop.
Test Plan: Havne't built a repro locally yet so not 100% sure this fixes it.
Reviewers: btrahan, mbishopim3, fabe
Reviewed By: mbishopim3, fabe
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T6880
Differential Revision: https://secure.phabricator.com/D11251
Summary: Fixes T6597, Uninstalled applications should not be clickable when searching "All Applications" in the Applications launcher
Test Plan: Navigate too /applications/query/all, uninstall an application, navigate back to all applications. Uninstalled application title should not be clickable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6597
Differential Revision: https://secure.phabricator.com/D11223
Summary: When updating the status of a task via commit, transaction should show responsible commit and status update if it was changed.
Test Plan: Push a commit "Fixes Txx", transaction should include status update and commit number.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6637
Differential Revision: https://secure.phabricator.com/D11230
Summary: Fixes T6862.
Test Plan: viewed a project list and saw disabled-style "Members" links as appropos
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6862
Differential Revision: https://secure.phabricator.com/D11229
Summary: Use `PhabricatorAuditEditor` instead of `PhabricatorEdgeEditor` when writing reverts edges. This ensures that a transaction is created in addition to the edge.
Test Plan: Reverted a commit and pushed to remote. Saw a row created in `phabricator_audit.audit_transaction_comment`. Interestingly, I can't actually see the transaction at http://phabricator.local/r${CALLSIGN}${REVERTED_COMMIT_HASH}.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11212
Summary: CLosed is a pretty important state and black tends to blend in a bit. This bumps to an alternate color to improve ability to scan and know state of objects.
Test Plan:
Review a number of closed objects. I will follow up with another diff on 'Archived' colors.
{F261895}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11222
Summary: Fixes T6608, though I'll also clean up the comment for PhutilStringTruncator in another diff. If I understand correctly, before T1191, MySQL column length was by character count and post T1191 its by byte count. Ergo, most of these changes are going from codepoint -> bytes. See test plan for complete list of what was and was not done.
Test Plan:
Thought very carefully about each callsite and made changes as appropos. "Display" means the string is clearly used for display-only purposes and correctly uses "glyph" already.
grep -rn PhutilUTF8StringTruncator *
applications/calendar/query/PhabricatorCalendarEventSearchEngine.php:217: ->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
applications/chatlog/controller/PhabricatorChatLogChannelLogController.php:111: $author = id(new PhutilUTF8StringTruncator()) -- display
applications/conduit/method/ConduitConnectConduitAPIMethod.php:62: $client_description = id(new PhutilUTF8StringTruncator()) -- was codepoint, changed to bytes
applications/conpherence/view/ConpherenceFileWidgetView.php:22: ->setFileName(id(new PhutilUTF8StringTruncator()) -- display
applications/differential/controller/DifferentialDiffViewController.php:65: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/event/DifferentialHovercardEventListener.php:69: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/parser/DifferentialCommitMessageParser.php:144: $short = id(new PhutilUTF8StringTruncator()) -- was glyphs, made to bytes
applications/differential/view/DifferentialLocalCommitsView.php:80: $summary = id(new PhutilUTF8StringTruncator()) -- display
applications/diffusion/controller/DiffusionBrowseFileController.php:686: id(new PhutilUTF8StringTruncator()) -- display
applications/feed/story/PhabricatorFeedStory.php:392: $text = id(new PhutilUTF8StringTruncator()) -- display, unless people are saving the results of renderSummary() somewhere...
applications/harbormaster/storage/build/HarbormasterBuild.php:216: $log_source = id(new PhutilUTF8StringTruncator()) -- was codepoints now bytes
applications/herald/storage/transcript/HeraldObjectTranscript.php:55: // NOTE: PhutilUTF8StringTruncator has huge runtime for giant strings. -- not applicable
applications/maniphest/export/ManiphestExcelDefaultFormat.php:107: id(new PhutilUTF8StringTruncator()) -- bytes
applications/metamta/storage/PhabricatorMetaMTAMail.php:587: $body = id(new PhutilUTF8StringTruncator()) -- bytes
applications/people/event/PhabricatorPeopleHovercardEventListener.php:62: id(new PhutilUTF8StringTruncator()) -- display
applications/phame/conduit/PhameCreatePostConduitAPIMethod.php:93: id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/pholio/storage/PholioTransaction.php:300: id(new PhutilUTF8StringTruncator()) -- display
applications/phortune/provider/PhortuneBalancedPaymentProvider.php:147: $charge_as = id(new PhutilUTF8StringTruncator()) -- bytes
applications/ponder/storage/PonderAnswerTransaction.php:86: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:267: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:276: id(new PhutilUTF8StringTruncator()) -- display
applications/repository/storage/PhabricatorRepositoryCommitData.php:43: $summary = id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:20: $data->setAuthorName(id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/slowvote/query/PhabricatorSlowvoteSearchEngine.php:158: $item->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php:317: $host = id(new PhutilUTF8StringTruncator()) -- bytes
view/form/control/AphrontFormPolicyControl.php:61: $policy_short_name = id(new PhutilUTF8StringTruncator()) -- glyphs, probably display only
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6608
Differential Revision: https://secure.phabricator.com/D11219
Summary: This appears to be a typo, identified by `ArcanistXHPASTLinter::LINT_DUPLICATE_SWITCH_CASE` (see D11171).
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11194
Summary: Ref T6861. Some discussion in IRC. The behavior of `sort` is somewhat broken when dealing with mixed types. In this particular case, we have both integers and strings.
Test Plan: @epriestley confirmed that this made the ordering of the Celerity map slightly-more-sane.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6861
Differential Revision: https://secure.phabricator.com/D11210
Summary:
Ref T1751. When a commit reverts another commit:
- Add an edge linking them;
- Show the edge in Diffusion.
Next steps are:
- If the reverted commit is associated with a Differential revision, leave a comment;
- Also leave a comment on the commit (no API yet);
- Also trigger an audit by the original commit's author.
Test Plan: Used `scripts/repository/reparse.php --message ...` to parse commits with revert language. Verified they appear correctly in Diffusion, and update Differential.
Reviewers: btrahan, epriestley
Reviewed By: btrahan, epriestley
Subscribers: Korvin, epriestley, cburroughs, joshuaspence, sascha-egerer, aran
Maniphest Tasks: T4896, T1751
Differential Revision: https://secure.phabricator.com/D5846
Summary: Show the full unit test name, including the namespace. Depends on D11208.
Test Plan: Inspected the "Table of Contents" of a diff created //with// D11208 and //without// D11208 applied.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11209
Summary: The default behavior was inadvertedly changed in D11074. This restores the original behavior.
Test Plan: Added a project reviewer to a diff, saw no inverse transaction recorded.
Reviewers: Krenair, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11181
Summary: This class is no longer used after D11125.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11170
Summary: These classes are no longer used after D10649.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11168
Summary: This class is no longer required after D6675.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11169
Summary: This class is no longer used after D6673.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11167
Summary: Ref T5655. Fixes T6849. This is another take on D11131, which was missing the DB migration and was reverted in rP7c4de0f6be77ddaea593e1f41ae27211ec179a55.
Test Plan: Ran `./bin/storage upgrade` and verified that the classes were renamed in the `phabricator_policy.policy` table.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6849, T5655
Differential Revision: https://secure.phabricator.com/D11166
Summary: This class is no longer used after D10965.
Test Plan: `grep`
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11133
Summary: Modernize remaining edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Browsed around and performed various actions include subscribing, unsubscribing and watching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11116
Summary: I was going to fix the variable name as it violates convention, but it is not used anyway.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11139
Summary:
Ref T2783.
This updates PhabricatorRepositoryManagementLookupUsersWorkflow to use ConduitCall to retrieve information about the commit.
Test Plan:
Ran `bin/repository lookup-users rTESTe9683b64d3283f0b2d355fdbf231bc918b5ac0ab --trace` and saw the information returned (by making a request to `diffusion.querycommits` as the omnipotent user, signed with the device key).
Mucked with `cluster.addresses` and saw requests rejected.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: Krenair, epriestley, Korvin
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10403
Summary:
Ref T2783. Ref T6706.
- Add `cluster.addresses`. This is a whitelist of CIDR blocks which define cluster hosts.
- When we recieve a request that has a cluster-based authentication token, require the cluster to be configured and require the remote address to be a cluster member before we accept it.
- This provides a general layer of security for these mechanisms.
- In particular, it means they do not work by default on unconfigured hosts.
- When cluster addresses are configured, and we receive a request //to// an address not on the list, reject it.
- This provides a general layer of security for getting the Ops side of cluster configuration correct.
- If cluster nodes have public IPs and are listening on them, we'll reject requests.
- Basically, this means that any requests which bypass the LB get rejected.
Test Plan:
- With addresses not configured, tried to make requests; rejected for using a cluster auth mechanism.
- With addresses configred wrong, tried to make requests; rejected for sending from (or to) an address outside of the cluster.
- With addresses configured correctly, made valid requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6706, T2783
Differential Revision: https://secure.phabricator.com/D11159
Summary:
Ref T2783. This is basically a more refined version of D10400, which churned a bit on things like SSH key storage, the actual way the signing protocol shook out, etc.
- When Phabricator tries to make an intra-cluster service call as the omnipotent user, sign it with the host's device key.
- Add `bin/almanac register` to say "this host is X device, identified by private key Y". This stores the keypair locally, adds the public key to Almanac, and trusts it.
Net effect is that once a host has been registered, the daemons can make calls to other nodes as the omnipotent user. This is primarily necessary so they can access repository API methods on remote hosts.
Test Plan:
- Ran `bin/almanac register` with various valid and invalid inputs.
- Verified keys get generated/added/stored properly.
- Made a device-signed cluster Conduit call.
- Made a normal old user-signed cluster Conduit call.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11158
Summary: This class is unused after D6679.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11149
Summary: This class is no longer used after D8168.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11152
Summary: This class is no longer required after D10869.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11154
Summary:
These didn't get translated quite right:
- We need to use `$total_count` because some languages have different words for 1, 2-3, and 4+ things (for example). So the strings might translate as:
- alincoln added a reviewer-one ...
- alincoln added reviewers-few ...
- alincoln added reviewers-many ...
- That is, while English has only "reviewer" and "reviewers", other languages have more plural forms, and "reviewer", "reviewers-few" and "reviewers-many" may be completely different words.
- In English, because we know we always have 2+ in this branch and the only special word is for 1, we can just drop this.
- Anyway, the %4$s stuff is counting assuming that $total_count is included in the string, so these were a off by one.
- See also D11160.
There a probably a couple more of these, but they should be easy enough to hunt down as they crop up.
Test Plan: Saw nice strings instead of empty strings, or invalid strings (after D11160).
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11162
Summary: The actual author here usually gets truncated by the extra text, which doesn't seem needed in most (all?) cases.
Test Plan: Look at list of recent commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11127
Summary:
Fixes T6692. Addresses two main issues:
- The write guard would sometimes not get disposed of on exception pathways, generating an unnecessary secondary error which was just a symptom of the original root error.
- This was generally confusing and reduced the quality of reports we received because users would report the symptomatic error sometimes instead of the real error.
- Instead, reflow the handling so that we always dispose of the write guard if we create one.
- If we missed the Controller-level error page generation (normally, a nice page with full CSS, etc), we'd jump straight to Startup-level error page generation (very basic plain text).
- A large class of errors occur too early or too late to be handled by Controller-level pages, but many of these errors are not fundamental, and the plain text page is excessively severe.
- Provide a mid-level simple HTML error page for errors which can't get full CSS, but also aren't so fundamental that we have no recourse but plain text.
Test Plan:
Mid-level errors now produce an intentional-looking error page:
{F259885}
Verified that setup errors still render properly.
@chad, feel free to tweak the exception page -- I just did a rough pass on it. Like the setup error stuff, it doesn't have Celerity, so we can't use `{$colors}` and no other CSS will be loaded.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, chad
Maniphest Tasks: T6692
Differential Revision: https://secure.phabricator.com/D11126
Summary: This reverts commit a896494ce5 which caused T6852
Test Plan: Check, for example, the position of the 'Save Account Settings' button
Reviewers: joshuaspence, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6852
Differential Revision: https://secure.phabricator.com/D11157
Summary:
Removes an unused PhabricatorFeedStory Parameter from all getTitleForFeed() and getApplicationTransactionTitleForFeed() functions.
Ref D11088 Ref T6545
Test Plan: ran all unit tests and viewed some dashboard feeds
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11146
Summary: This class is no longer used after D7370.
Test Plan: `arc lint` and `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11132
Summary: This class is no longer used after D7370.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11129
Summary: This class is no longer required after D10792.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11134
Summary: This class is no longer used after D7585.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11135
Summary: Modernize Dashboard edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Attached a panel to a dashboard, observed the expected comment in the transaction view (both ways).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11114
Summary: Modernize Pholio edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Attached a mock to a task, observed the expected comment in the transaction view (both ways).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11113
Summary: Modernize Project edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Add a member to a project, saw new rows in the `phabricator_project.edge` and `phabricator_user.edge` tables.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11111
Summary: This method has been deprecated for a long time (see D2887 and D6336).
Test Plan:
```lang=bash
> echo '{}' | arc --conduit-uri=http://phabricator.local call-conduit 'maniphest.find'
Waiting for JSON parameters on stdin...
{"error":"ERR-CONDUIT-CALL","errorMessage":"ERR-CONDUIT-CALL: ","response":null}
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11117
Summary: In D10027, the `getName()` method was added to `PhabricatorApplication` to //allow// applications to explicitly declare their names rather than relying on the name being inferred from the name of the `PhabricatorApplication` subclass. I feel that sufficient time has passed such that we can //require// applications to explicitly declare their names.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11119
Summary: I assume this can be removed now?
Test Plan: Not tested, hmm...
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11123
Summary: This class is unused after D6488, maybe we should remove it?
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11124
Summary:
Ref T4245 Make repo callsigns optional
This is far from done and still very ugly. I'm just submitting it to check if i'm solving this in the right places.
Right now there's three places with duplicate code and building the identifierMap in the CommitQuery is very ugly.
If we only want to support this in the user frontend then i could hack it into the Markup rule itself and not touch the CommitQuery. Even uglier but more limited in scope...
Generally this approach will need a lot of "check this first and then try the other" in a few places.
I could move the Repository queries into a specialised PhabricatorRepositoryQuery method (withCallsignOrID) but i'm not sure about that.
Test Plan:
- phid.lookup works with R1 and rTEST (which is the same repo)
- R1 and rTEST euqally work in remarkup (tested in comments).
- Reviewed the following syntax also all works:
rTEST
rTESTd773137a7cb9
rTEST:d773137a7cb9
R1
R1:d773137a7cb9
d773137a7cb9
{rTEST}
{rTESTd773137a7cb9}
{rTEST:d773137a7cb9}
{R1}
{R1:d773137a7cb9}
{d773137a7cb9}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D11050
Summary:
Ref T4411
I'm not quite sure if this is the right place for this as it will be difficult to provide proper user feedback of why we removed a particular subscriber.
Is the ApplicationTransactionEditor generally the right place to extract mentioned phids in comments?
On the other hand in some cases we cannot really give user feedback why a user was not subscribed (e.g.: commits & diffs)
Adding a diff to a repo where the user mentioned has no view permissions the subscriber is currently still added. Still would have to find where this is donet...
Any other places?
Unrelated: Is there any way to remove a subscriber from a commit/audit ?
Test Plan:
- Edited tasks with the mentioned user having view permissions to this specific task and without
- Raised concern with a commit and commented on the audit with the user having view permissions to the repo and without
- Added a commit to a repo with and without the mentioned user having permissions
- Mention a user in a task & commit comment with and without permissions
- Mentioning a user in a diff description & comments with and without permissions to the specific diff
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T4411
Differential Revision: https://secure.phabricator.com/D11049
Summary:
Shows a timeline of all modified settings Fixes T6545
Will show all settings (no pagination, should be not so difficult to add if needed but most installs won't have hundreds of settings changes)
I'm not happy by how the PhabricatorConfigTransaction object is instructed to render the config keys but i don't see any other reasonable way.
We could always show the keys though.
Test Plan: Changed settings and called the history page
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11088
Summary: These classes are named differently from other `PhabricatorEdgeType` subclasses. Rename them for consistency.
Test Plan: I would expect the linter to complain if I missed anything.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11112
Summary: Modernize Differential edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Krenair, epriestley
Differential Revision: https://secure.phabricator.com/D11074
Summary: This is a typo from D11045.
Test Plan: I haven't actually tested this, but the tests from D11045 should apply here.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11108
Summary: These are no longer required after D7076.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11107
Summary: These strings are no longer required after D10678.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11106
Summary: These are no longer required after D11032.
Test Plan: `grep`
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11104
Summary: Modernize Ponder edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: I couldn't actually figure out how to get these strings to show up anywhere.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Krenair, chad, epriestley
Differential Revision: https://secure.phabricator.com/D11083
Summary: Modernize Legalpad edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan:
# Created a Herald rule to require legal signatures on all diffs.
# Created a new diff.
# Saw the transaction string appear correctly.
I wasn't able to check the inverse transaction because there is none. Also, I couldn't see any text on the feed (presumably, transactions authored by Herald do not generate feed items)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Krenair, chad, epriestley
Differential Revision: https://secure.phabricator.com/D11082
Summary:
Ref T2783. This method is kind of goofballs:
- We send a big list of paths to it.
- It sends back a giant blob of HTML.
Instead, just figure out the path we want locally, then fetch the content with `diffusion.filecontentquery`.
Test Plan:
- Viewed main view and directory view, saw a README.
- See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11099
Summary:
Ref T2783. Currently, the repository edit page does some checks agaisnt the local system to look for binaries and files on disk. These checks don't make sense in a cluster environment.
Ideally, we could make a Conduit call to the host (e.g., add something like `diffusion.querysetupstatus`) to do these checks, but since they're pretty basic config things and cluster installs are advanced, it doesn't seem super worthwhile for now.
Test Plan: Saw fewer checks in a cluster repo.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11102
Summary:
Ref T2783. When repository services are defined, skip the check for local repository storage.
In particular, in the Phacility cluster, this path won't exist on web nodes, but nothing will ever try to access it.
Test Plan: Verified new branch gets hit with cluster services defined.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11100
Summary: Fixes T6838. We use a special transaction type for merging, but don't handle it when figuring out mail tags.
Test Plan: Verified merge mail picks up the `maniphest-status` tag.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6838
Differential Revision: https://secure.phabricator.com/D11101
Summary: T5549
Test Plan: Set edit policy on paste, check that only users meeting the policy requirements can edit it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T5549
Differential Revision: https://secure.phabricator.com/D11097
Summary: Ref T5402.
Test Plan:
- Queried archived tasks.
- Grepped for use sites and verified no other callsites are order-sensitive.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11089
Summary: This is a fake hash of many 0s which ends up being a bad link. Detect the fake hash and don't print a link. Fixes T6826.
Test Plan: looked at push log and no longer saw a many 0 entry for the first old ref.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6826
Differential Revision: https://secure.phabricator.com/D11096
Summary: Fixes T6834. Without this asort() call, repositories are ordered by ID, which is arbitrary to the user. This sorts them alphabetically by callsign.
Test Plan: edited a package and saw repos sorted by call sign
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6834
Differential Revision: https://secure.phabricator.com/D11095
Summary: Ref T5402, T6238. Pull this out into a class so the Instances app can embed task views.
Test Plan: Loaded `/daemon/` and examined the content in the tables.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6238, T5402
Differential Revision: https://secure.phabricator.com/D11090
Summary: This is dead code.
Test Plan: These edge types don't actually seem to be used?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11076
Summary: Fix a few minor lint issues.
Test Plan: Ran `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11059
Summary:
Since the default query will sort on this when no query string is
attached we want to make sure the property at least exists.
Otherwise Elasticsearch yells at you: "No mapping found for [dateCreated]
in order to sort on" when you try to search for documents that haven't
been indexed yet.
Test Plan:
Searched for Mocks and Initiatives (no such documents exist in my index)
and got the error. After patching and reinitializing the index, the error
during querying went away.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11056
Summary: I don't have `hg` yet on my new laptop; we should just skip tests if the user is missing binaries. Add a convenience method to do this.
Test Plan: Got clean `arc unit --everything` with no `hg` installed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11051
Summary: Fixes T6795. Fixes T6813. We can give more tailored instructions for surplus schemata than we currently do, and provide more information on resolving them.
Test Plan:
- Ran `storage adjust` with just surplus schemata (friendly warning).
- Ran `storage adjust` with surplus schemata and other serious errors (more severe error).
- Read document.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6795, T6813
Differential Revision: https://secure.phabricator.com/D11054
Summary:
Ref T6817. Ref T5726. These both bypass policy checks, and would allow an attacker who gains control of an administrative account to enable public feed, then view feed stories they could not normally see; or enable feed.http-hooks, then read the posted text.
In the longer term I'd like to remove `feed.public` completely (possibly providing API alternatives, if necessary).
Test Plan: Looked at options in web UI and saw them locked.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6817, T5726
Differential Revision: https://secure.phabricator.com/D11046
Summary: Ref T5402. This more or less "fixes" it but there's probably some polish to do?
Test Plan:
stopped and started daemons. error logs look good.
ran bin/storage upgrade. noted that `adjust` added the appropriate indices for active and archive task.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11044
Summary: Ref T5402. This cleans up some code and sets us up to use this sort of data more easily later.
Test Plan: viewed the daemon console from the web and the log of a specific archived daemon. both looked good. for other callsites looked really, really carefully.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11042
Summary: See rP2fedb6f941d8. We might need a more general version of this since we do some `sudo` stuff elsewhere, but at least on my machine `sudo -n` exits with code 0 when the target user exists but needs a password.
Test Plan:
- Tried to run daemons as root, with no automatic sudo to root. Got a bad result before (phd believed it had executed the daemons) and a good result afterward (phd recognized that sudo failed).
- Tried to run daemons from root, as a non-root user. Got a good result in both cases.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: fabe, epriestley
Differential Revision: https://secure.phabricator.com/D11041
Summary: Fixes T6807. This page shows edit history and has edit options, but is not an edit interface itself per se. Allowing public access is consistent with other interfaces/applications.
Test Plan: Viewed a public project's history page as a public user.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6807
Differential Revision: https://secure.phabricator.com/D11039
Summary:
Fixes T5196
If no phd.user is configured the behaviour is unchanged besides printing a warning when run as root (Usually i would add an exit(1) here but that would break existing installs who do that).
If phd.user is set and the current user is root it will run the daemon as: su USER -c "command" (I'm not sure if this works for every platform needed)
Otherwise it will refuse to start if configured and current user mismatch.
Test Plan: Stopped & Started phd daemon with various users and different phd.user settings including root
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: vinzent, epriestley
Maniphest Tasks: T5196
Differential Revision: https://secure.phabricator.com/D11036
Summary: Show the php.ini setting for max upload or alterantively the in phabricator configured one. Fixes T6663
Test Plan: changed php.ini and alternatively phabricator file upload size settings to minimal values and try to upload a larger file
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, btrahan, chad, epriestley
Maniphest Tasks: T6663
Differential Revision: https://secure.phabricator.com/D11031
Summary:
When the index does not exist and auto_create_index isn't
enabled, running ./bin/index results in a failure. That's
T5990
Instead create an index properly. This also allows us to do
nice things like do a proper mapping and analysis like for
substring matching like outlined by @fabe in T6552.
Test Plan:
Deleted and created index multiple times to verify
proper index creation and usage.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, manybubbles, chasemp, fabe, epriestley
Differential Revision: https://secure.phabricator.com/D10955
Summary:
It's like query_string but fails a little nicer on bad
input. It also allows for limited Lucene syntax; notably
exact string matches with quotation marks.
Fixes T6780
Test Plan:
Tested multiple query constructions, including exact string
matching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T6780
Differential Revision: https://secure.phabricator.com/D11011
Summary: Ref: T6747 add a setup warning if mysql is configured with the default boolean syntax
Test Plan: Run setup checks with default mysql config and then add the desired config option, restart and check again.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T6747
Differential Revision: https://secure.phabricator.com/D11030
Summary: Ref T5833. Ref T6238. These are general capabilities which are particularly useful for synchronizing cluster specifications to instances.
Test Plan:
- Synchronized networks, devices, interfaces, services, bindings and properties to a managed instance.
- Used typeahead.
- Destroyed networks, devices, and services. Saw interfaces and bindings destroyed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238, T5833
Differential Revision: https://secure.phabricator.com/D11024
Summary: Removes the docs sprite in Conpherence with FontAwesome, adds additional icons. Unsure what happens if someone customized this config option.
Test Plan: Added images and files to a Conpherence, saw new icons.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11028
Summary: Fixes an exception on Conpherence.
Test Plan: Use Conpherence. Grep for other locations.
Reviewers: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11027
Summary: Removes unused payments sprite and code, also some unused conpherence generated images. We use images in login (and could use FontAwesome, maybe).
Test Plan: grep codebase, pull up uiexamples icons page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11025
Summary: These were refactored out a while ago
Test Plan: Grep codebase, use Conpherence on desktop, mobile.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11023
Summary:
Ref T4209. Ref T6240. Ref T6238. See D10401 for original discussion.
On OSX, `ssh-keygen` doesn't support PKCS8:
- When we hit an issue with this, raise a more tailored message about it.
- Allow the user to work around the problem with `auth cache-pkcs8 ...`, providing reasonable guidance / warnings.
In practice, this only really matters very much for one key, which I'm just going to make the services extension cache automatically. So it's sort of moot, but good to have around for weird cases and to make testing easier.
Test Plan: Hit error, cached key, got clean asymmetric auth.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4209, T6240, T6238
Differential Revision: https://secure.phabricator.com/D11021
Summary: Fixes T6790. Turn the old method into "new" (old signature) and "newEphemeral". Deploy "newEphemeral" as many places as possible; basically places we are not in the Differential application *and* have no intentions of ever saving the diff. These callsites are also all places we are just trying to get some changesets at the end of the day.
Test Plan: set differential application policy to 'administrators only'. viewed a commit in diffusion and it worked without any errors! i'm just using my thinkin' noodle on the other code paths.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6790
Differential Revision: https://secure.phabricator.com/D11020
Summary: Ref T5833. Ref T6238. This will probably be useful somewhere in the upstream eventually, and is materially useful in the Instances application right now.
Test Plan: tippy typey typey
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238, T5833
Differential Revision: https://secure.phabricator.com/D11016
Summary: database migration + drop old view code. Fixes T5604.
Test Plan: grepped src/ for TYPE_CCS (no hits); viewed some tasks with old cc transactions and noted they still rendered correctly post data conversion
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D11015
Summary: The stacked version of property lists is supposed to display like a normal definition list.
Test Plan: Test the stacked layout in Herald.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11014
Summary:
Fixes T6741. This allows Almanac services to be locked from the CLI. Locked services (and their bindings, interfaces and devices) can not be edited. This serves two similar use cases:
- For normal installs, you can protect cluster configuration from an attacker who compromises an account (or generally harden services which are intended to be difficult to edit).
- For Phacility, we can lock externally-managed instance cluster configuration without having to pull any spooky tricks.
Test Plan:
- Locked and unlocked services.
- Verified locking a service locks connected properties, bindings, binding properties, interfaces, devices, and device properties.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6741
Differential Revision: https://secure.phabricator.com/D11006
Summary:
Ref T2783. When creating a new repository, test for cluster services. If cluster services are available, allocate on a random open service.
Show the service that repositories are allocated on.
Test Plan: Created a new repository, saw it allocate onto an available cluster service.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11003
Summary:
...except the transaction class itself, which still needs some knowledge of these transactions for older installs.
Ref T5245. T5604 and T5245 are now in a similar place -- there's an unknown set of bugs introduced from my changes and there's still old display code lying around with some old transactions in the database. I'll stomp out the bugs if / when they surface and data migration is up next.
This revision also adds a "TransactionPreviewString" method to the edge objects so that we can have a prettier "Bob edited associated projects." preview of this transaction.
Test Plan: added a project from task detail and saw correct preview throughout process with correct project added. bulk removed a project from some tasks. added a project from the edit details pane.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11013
Summary: I find this easier to read
Test Plan: Hover over tooltip area
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11012
Summary: Ref T5245. This is some of the associated cleanup there.
Test Plan:
foreach ManiphestTaskQuery site, I made the change (or not) and tested as follows:
=== Call sites where added needProjectPHIDs ===
- PhabricatorHomeMainController - loaded the home page
- ManiphestBatchEditController - batch edited some tasks (added a project)
- ManiphestConduitAPIMethod - tested implicitly when tested ManiphestUpdateConduitAPIMethod
- ManiphestInfoConduitAPIMethod - used the method via conduit console with input id : 1
- ManiphestQueryConduitAPIMethod - used the method via conduit console with input ids : [1, 2]
- ManiphestUpdateConduitAPIMethod - used the method via conduit with input id : 1 and comment : “asdasds"
- ManiphestReportController - viewed “By User” and “By Project”
- ManiphestSubpriorityController - changed the priority of a task via a drag on manphest home
- ManiphestTaskMailReceiver - updated Task 1 via bin/mail receive-test with a comment that is the README
- ManiphestTaskSearchEngine - loaded Manifest home page
- ManiphestTaskEditController - edited a task
- ManiphestTransactionEditor - closed a blocking task
- ManiphestTransactionSaveController - commented on a task
- PhabricatorProjectProfileController - viewed project with id of 1 that has a few tasks in it
- PhabricatorSearchAttachController - merged tasks together
- DifferentialTransactionEditor - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
- PhabricatorRepositoryCommitMessageParserWorker - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
=== Calls sites where *did not* add needProjectPHIDs (they do not appear in this revision) ===
- PhabricatorManiphestApplication - loaded the home page
- ManiphestGetTaskTransactionsConduitAPIMethod - used the method via conduit console with input ids : [1, 2] ManiphestTaskDetailController - viewed a task with and without associated projects; finished workflow creating a task with a parent
- ManiphestTransactionPreviewController - verified transaction preview showed up properly
- PhabricatorProjectBoardViewController - viewed a board
- PhabricatorProjectMoveController - moved a task around
- ManiphestRemarkupRule - made a task reference like {T123}
- ManiphestTaskQuery - executed a custom query for all tasks with page size of 2 and paginated through some tasks
- ManiphestTaskPHIDType - nothing random seems broken? =D
=== Call sites where had to do something funky ===
- ManiphestHovercardEventListener - loaded hover cards from task mentions
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11004
Summary: Using Project lists on mobile is missing the query menu, this adds it.
Test Plan: test mobile layout, click menu, see query list
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11010
Summary: Makes the header text clickable to the full results.
Test Plan: Click on a few queries.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11008
Summary: Ref T6741. As an optimization, we omit policy checks on ObjectQuery, but need to retain them if the query requests non-view permissions. This primarily affected Almanac properties.
Test Plan: Almanac properties now do full policy checks on ObjectQuery loads.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6741
Differential Revision: https://secure.phabricator.com/D11005
Summary: Ref T2783. In Diffusion -> Edit Repository, we currently have a section called "Local" with options about where the repository is stored. The current name is misleading in a cluster environment, where storage may not actually be local. Shortly, this will also have an option for cluster storage. Call this "Storage" instead.
Test Plan: Edited a repository and poked around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11001
Summary:
Ref T5833. This allows Almanac ServiceTypes to define default properties for a service, which show up in the UI and are more easily editable.
Overall, this makes it much easier to make structured/usable/consistent service records: you can check a checkbox that says "prevent new allocations" instead of needing to know the meaning of a key.
Test Plan: {F251593}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10996
Summary:
Ref T5833. This allows services to be typed, to distinguish between different kinds of services. This makes a few things easier:
- It's easier for clients to select the services they're interested in (see note in T5873 about Phacility). This isn't a full-power solution, but gets is some of the way there.
- It's easier to set appropriate permissions around when modifications to the Phabricator cluster are allowed. These service nodes need to be demarcated as special in some way no matter what (see T6741). This also defines a new policy for users who are permitted to create services.
- It's easier to browse/review/understand services.
- Future diffs will allow ServiceTypes to specify more service structure (for example, default properties) to make it easier to configure services correctly. Instead of a free-for-all, you'll get a useful list of things that consumers of the service expect to read.
The "custom" service type allows unstructured/freeform services to be created.
Test Plan:
- Created a new service (and hit error cases).
- Edited an existing service.
- Saw service types on list and detail views.
- Poked around new permission stuff.
- Ran `almanac.queryservices` with service class specification.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10995
Summary:
Ref T6742. Root cause of the issue:
- Daemon was running on a machine with a very long host name, which produced a lease name which was longer than 64 characters.
- MySQL wasn't set in STRICT_ALL_TABLES.
- The daemon would `UPDATE .. SET leaseOwner = <very long string>` to lock a task, and MySQL would silently truncate.
- The daemon would then try to select the locked task, but fail, because there's no matching lease owner.
To resolve this, use only the first 32 characters of the hostname. See IRC for more discussion.
Test Plan: Will confirm with reporter.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6742
Differential Revision: https://secure.phabricator.com/D10998
Summary: We didn't load enough data for this case for the custom view class Pholio uses. Fixes T6770. Re-jiggers the signature of the loadImages function in the PholioMockQuery to get there so as to not duplicate any business logic.
Test Plan: made a pholio mock with lots of inline comments. pre-patch "show older" fatals and post-patch "show older" works
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6770
Differential Revision: https://secure.phabricator.com/D11002
Summary: If no Diviner groups are defined that this line currently throws a fatal exception... make this a little more safe.
Test Plan: Applied to our install, no more exceptions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10999
Summary:
Ref T5955. Ref T2783.
- Removes the "temporary" type. I was going to use this for T3628 but it started taking more time than I wanted to spend on it.
- Add a "cluster" type, which is an internal-only token type used within a cluster. This token value is never shown to the user.
- Automatically generate, use, and cycle cluster tokens.
Test Plan:
- Diffusion (mostly) works with a repository configured to use a remote service.
- Saw cluster tokens generate; terminated a cluster token and saw it regenerate.
- Viewed cluster token in settings panel and saw nice explanatory text instead, as expected (we might just hide these eventually).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783, T5955
Differential Revision: https://secure.phabricator.com/D10990
Summary:
Ref T5955.
- Add `conduit.getcapabilities` to help arc (and other clients) determine formats, protocols, etc., the server supports.
- Fixes T3117. Add a more modern version of the handshake workflow that allows all generated tokens to remain valid for an hour.
- Generally, add a CLI token type. This token type expires after an hour when generated, then becomes permanent if used.
Test Plan:
- See D10988.
- Ran `conduit.getcapabilities` and inspected output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3117, T5955
Differential Revision: https://secure.phabricator.com/D10989
Summary:
- Ref T5955. Accept the tokens introduced in D10985 as an authentication token.
- Ref T3628. Permit simple `curl`-compatible decoding of parameters.
Test Plan:
- Ran some sensible `curl` API commands:
```
epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/user.whoami?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk" ; echo
{"result":{"phid":"PHID-USER-cvfydnwadpdj7vdon36z","userName":"admin","realName":"asdf","image":"http:\/\/local.phacility.com\/res\/1410737307T\/phabricator\/3eb28cd9\/rsrc\/image\/avatar.png","uri":"http:\/\/local.phacility.com\/p\/admin\/","roles":["admin","verified","approved","activated"]},"error_code":null,"error_info":null}
```
```
epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/differential.query?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk&ids[]=1" ; echo
{"result":[{"id":"1","phid":"PHID-DREV-v3a67ixww3ccg5lqbxee","title":"zxcb","uri":"http:\/\/local.phacility.com\/D1","dateCreated":"1418405590","dateModified":"1418405590","authorPHID":"PHID-USER-cvfydnwadpdj7vdon36z","status":"0","statusName":"Needs Review","branch":null,"summary":"","testPlan":"zxcb","lineCount":"6","activeDiffPHID":"PHID-DIFF-pzbtc5rw6pe5j2kxtlr2","diffs":["1"],"commits":[],"reviewers":[],"ccs":[],"hashes":[],"auxiliary":{"phabricator:projects":[],"phabricator:depends-on":[],"organization.sqlmigration":null},"arcanistProjectPHID":null,"repositoryPHID":null,"sourcePath":null}],"error_code":null,"error_info":null}
```
- Ran older-style commands like `arc list` against the local install.
- Ran commands via web console.
- Added and ran a unit test to make sure nothing is using forbidden parameter names.
- Terminated a token and verified it no longer works.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3628, T5955
Differential Revision: https://secure.phabricator.com/D10986
Summary:
Ref T5955. Summary of intended changes:
**Improve Granularity of Authorization**: Currently, users have one Conduit Certificate. This isn't very flexible, and means that you can't ever generate an API token with limited permissions or IP block controls (see T6706). This moves toward a world where you can generate multiple tokens, revoke them individually, and assign disparate privileges to them.
**Standardize Token Management**: This moves Conduit to work the same way that sessions, OAuth authorizations, and temporary tokens already work, instead of being this crazy bizarre mess.
**Make Authentication Faster**: Authentication currently requires a handshake (conduit.connect) to establish a session, like the web UI. This is unnecessary from a security point of view and puts an extra round trip in front of all Conduit activity. Essentially no other API anywhere works like this.
**Make Authentication Simpler**: The handshake is complex, and involves deriving hashes. The session is also complex, and creates issues like T4377. Handshake and session management require different inputs.
**Make Token Management Simpler**: The certificate is this huge long thing right now, which is not necessary from a security perspective. There are separate Arcanist handshake tokens, but they have a different set of issues. We can move forward to a token management world where neither of these problems exist.
**Lower Protocol Barrier**: The simplest possible API client is very complex right now. It should be `curl`. Simplifying authentication is a necessary step toward this.
**Unblock T2783**: T2783 is blocked on nodes in the cluster making authenticated API calls to other nodes. This provides a simpler way forward than the handshake mess (or enormous-hack-mess) which would currently be required.
Test Plan:
- Generated tokens.
- Generated tokens for a bot account.
- Terminated tokens (and for a bot account).
- Terminated all tokens (and for a bot account).
- Ran GC and saw it reap all the expired tokens.
NOTE: These tokens can not actually be used to authenticate yet!
{F249658}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D10985
Summary:
Two goals:
- If an attacker compromises an administrator account (without compromising the host itself), they can currently take advantage of vulnerabilities in prototype applications by enabling the applications, then exploiting the vulnerability. Locking this option requires CLI access to enable prototypes, so installs which do not have prototypes enabled have no exposure to security issues in prototype applications.
- Making this very slightly harder to enable is probably a good thing, given the state of the world and support.
Test Plan: Verified that web UI shows the value is locked and instructs the user to update via the CLI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10993
Summary: Via HackerOne. An attacker with access to both Phame and the filesystem could potentially load a skin that lives outside of the configured skin directories, because we had insufficient checks on the actual skin at load time.
Test Plan: Attempted to build a blog with an invalid skin; got an exception instead of a mis-load of a sketchy skin.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10992
Summary: Fixes T6748. This just didn't get aligned when CCs became a modern transaction.
Test Plan: Added a CC to a task, used `bin/mail show-outbound` to verify it showed up as a CC tag.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6748
Differential Revision: https://secure.phabricator.com/D10991
Summary:
Ref T2783. This is primarily exploratory and just figuring out what we're blocked on:
- Allow a Repository to be bound to a Service. The Service may eventually define multiple read/write nodes, etc.
- There's no UI to do this binding yet, you have to touch the database manually.
- If a repository is bound to a Service, effect Conduit calls via calls to the remote service instead of executing them in-process.
- These don't actually work yet since there's no authentication (see T5955).
Test Plan:
- Made a nice Service with a nice Binding to a nice Interface on a nice Device.
- Force-associated a repository with the service using a raw MySQL query.
- Saw Phabricator try to make a remote call to the service (on localhost) and fail because of missing auth stuff.
- Also ran `almanac.queryservices`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10982
Summary: Fixes T6595. This diff has two issues as is... 1) the differential data fetching is pretty cheesey, but it looks like we can't just issue three separate databases to get the right data? 2) the translations break, since I am turning this into a string (and not an int) so the whole pluralization bit fails. I think 1 is okay as is and 2 needs to be fixed though I am not sure how to best do that...
Test Plan: loaded home page and it looked nice...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6595
Differential Revision: https://secure.phabricator.com/D10979
Summary:
Fixes T6651, T6682. Since policy is defined by ancestry, you can't make things outside the core tree.
An alternative fix would be to automagically stub everything in these cases. This has potential negative policy implications - consider making a public document with several levels of depth that automagically stubs out its ancestry as public - and additionally the PhabricatorApplicationTransactionEditor framework would make this very tricky code (i.e. you are expected to validateTransactions in said hook *and* return an error if things aren't valid and not do some automagic stubbing, etc.)
Test Plan: tried to move a doc from location/that/exists to locationz/thatz/dontz/existz/ and got an error message with links to each missing doc. tried to create a doc at locatonz/thatz/dontz/existsz/ and got an error message with links to each missing doc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6682, T6651
Differential Revision: https://secure.phabricator.com/D10978
Summary: Fixes T6734. This is a very generic fix, which basically attaches the subscribers if necessary. This seems like a good idea given there's some crazy generic code doing this sort of thing? This would end up being a new pattern for these types of objects that may be loaded by a general object query but then get some editor action against them.
Test Plan: I can't actually reproduce this in my sandbox so I'll verify live again.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6734
Differential Revision: https://secure.phabricator.com/D10976
Summary:
There's a comma to the lower-left of my profile picture here:
{F248962}
This is on a page like https://secure.phabricator.com/F248948
What's happening is that some `render()` method is returning a valid result like `array($stuff, null)`. This is getting passed to JS as an array, which is implicitly `join()`'ing it into a string, adding a comma.
Instead, make sure we render these to strings on the server side before shipping them to the client.
Test Plan: No more comma on file previews.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10974
Summary: Ref T5604. Found this trying to open T5604 live. Basically this internal query needs the needSubscriberPHID set to true.
Test Plan: doing it live
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D10975
Summary: Fixes T6731. I don't really understand the intent behind the two view classes here, but to get this to work I need to pass yet more data to the lower-level class.
Test Plan: Viewed a task with many comments. Clicked "show older". Quoted everything I could. Verified for each quote that it quoted correctly, inlcuding linking to the prior transaction.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6731
Differential Revision: https://secure.phabricator.com/D10973
Summary: (Needed a clean branch). Moves the field up and renames to Query
Test Plan: Visit Maniphest Search, see new field, test a query
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10971
Summary: Fixes T6727. Repro is: mention a task on another task, in a comment.
The inverse edge editor applying the "alincoln mentioned this in <other task>" transaction doesn't have enough data to execute Herald rules.
Just don't try to execute the rules, since they don't make much sesne from a product perspective and are tricky from a technical perspective.
Test Plan: Commented on `T1` with `T2` in comment body and a Herald rule that examines subscribers.
Reviewers: btrahan
NOTE: Cowboy committing this since any task mention fatals.
Summary: Fixes T5604. This should fix some random bugs, lets us move forward more easily, and all that good stuff about killing code debt.
Test Plan:
- Conduit method maniphest.createtask
- verified creating user subscribed
- verified subscription transaction
- Conduit method maniphest.update
- verified subscribers set as specified to ccPHIDs parameter
- verified subscription transaction
- Herald
- verified herald rule to add subscriber worked
- verified no subscribers removed accidentally
- edit controller
- test create and verify author gets added IFF they put themselves in subscribers control box
- test update gets set to exactly what user enters
- lipsum generator'd tasks work
- bulk add subscribers works
- bulk remove subscriber works
- detail controller
- added myself by leaving a comment
- added another user via explicit action
- added another user via implicit mention
- task merge via search attach controller
- mail reply handler
- add subscriber via ./bin/mail receive-test
- unsubscribe via ./bin/mail receive-test
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D10965
Summary: I think this is what you're after?
Test Plan: clicky clicky
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10966
Summary: Fixes T6598, "Primary Hashtag" field should only be visible in edit mode of existing projects.
Test Plan: Create project, "Primary Hashtag" field should be hidden. Edit an existing project, "Primary Hashtag" field should appear above "Additional Hashtags" as before.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6598
Differential Revision: https://secure.phabricator.com/D10964
Summary:
Ref T2783. ConduitCall currently has logic to pick a random remote server, but this is ultimately not appropriate: we always want to send requests to a specific server. For example, we want to send repository requests to a server which has that repository locally. The repository tier is not homogenous, so we can't do this below the call level.
Make ConduitCall always-local; logic above it will select ConduitCall for an in-process request or do service selection for an off-host request via ConduitClient.
Test Plan:
- Browsed some pages using ConduitCall, everything worked.
- Grepped for removed stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10959
Summary: This adds back the title to the header link and scans through the codebase for instances where
Test Plan: Tested as many ObjectItemLists as I could find (each app homepage), there may be outliers, but can resolve those individually.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10961
Summary: Ref T6723. This allows you to hover over truncated headers and get the full text if needed.
Test Plan:
Hover over header, see full title
{F248197}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6723
Differential Revision: https://secure.phabricator.com/D10958
Summary: Fixes T6562, Title/Description querying for Passphrase
Test Plan: Open Passphrase, open advanced queries, enter a title and/or description. Search results should show credentials matching the search.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6562
Differential Revision: https://secure.phabricator.com/D10953
Summary: Ref T6713. We were dropping latest transaction ID. This should fix the "easy" part of T6713.
Test Plan: tried to add participants and it worked! (removing participants only allows yourself, but that worked too.)
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6713
Differential Revision: https://secure.phabricator.com/D10952
Summary: Fixes T6719. At some point, we added automagical subscriptions via @mentions, and these were failing in project descriptions from a lack of an implementation in the editor. Said "implementation" is to do nothing, but it needs to be there nonetheless.
Test Plan: updated a project mentioning someone in the description and it worked. also saw a 'subscriber added' transaction
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6719
Differential Revision: https://secure.phabricator.com/D10951
Summary: Cleans up spacing, updates to fonts instead of images. Fixed some mobile issues.
Test Plan:
Test with and without counts on desktop, tablet, mobile. Test layout in FF, Chrome, IE.
{F246745}
{F246746}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10948
Summary: Fixes T6664, clicking search icon in empty search field should link to advanced search
Test Plan: navigate to home page, click search icon or click into search box and hit enter. Advanced search page should open.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6664
Differential Revision: https://secure.phabricator.com/D10947
Summary:
The `$timeline` variable is undefined. I was seeing the following error in the logs:
```
EXCEPTION: (RuntimeException) Undefined variable: timeline at [<phutil>/src/error/PhutilErrorHandler.php:210]
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/harbormaster/controller/HarbormasterStepEditController.php:205]
#1 HarbormasterStepEditController::processRequest() called at [<phabricator>/src/aphront/AphrontController.php:33]
#2 AphrontController::handleRequest(AphrontRequest) called at [<phabricator>/webroot/index.php:103]
```
Test Plan: Created a build step without a fatal error.
Reviewers: btrahan, hach-que, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10941
Summary:
Fixes T6702. Ref T3554. Currently, tasks can be cancelled, retried and freed from the web UI by any logged in user.
This isn't appreciably dangerous (I can't come up with a way that a user could do anything security-affecting), but I think I probably intended this to be admin-only, but these actions should move to the CLI anyway.
Move them to the CLI. Lay some groundwork for some future `bin/worker cancel --class SomeTaskClass`, but don't implement that yet.
Test Plan: Used `cancel`, `retry` and `free` from the CLI. Hit all the error/success states.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3554, T6702
Differential Revision: https://secure.phabricator.com/D10939
Summary: Ref T3669. Probably. Adds a yellow warning at the top of the Diff View and makes the comment draft icon yellow on lists of revisions.
Test Plan:
Test a diff with many warnings, see warning. Test a diff with draft comments, see warning. Test new icon in list view.
{F230133}
{F230134}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3669
Differential Revision: https://secure.phabricator.com/D10789
Summary: Fixes T6699. We need to "loadInlineComments" consistently, though for an unexpected reason - this mutates the $changesets to include all $changesets that have an associated inline comment, which is necessary to make them render properly.
Test Plan: Took a diff with inline comments and updated it, noting the inline comments disappeared. applied this patch and the inlines reappeared.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6699
Differential Revision: https://secure.phabricator.com/D10935
Summary: Change icon for Settings app to more match previous. Also align plus icon a little better.
Test Plan: Lots of staring.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10934
Summary: Fixes T6694. Ref T4712. Turns out the logic here was slightly incorrect; we don't want to use the id of the last thing we hid but rather the first thing we show. I had garbage test data ("asdsadsadsa", etc) I guess so I didn't notice this.
Test Plan: made a new task where user a and user b alternated 3 comments each, cooperatively numbering them from 1 - 20. as both users, showed older transactions. pre-patch the issue described in T6694 occurred and post patch I saw the entire counting sequence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712, T6694
Differential Revision: https://secure.phabricator.com/D10933
Summary: we still need to be pager-sensitive, but otherwise this "show all" stuff is dead, dead dead...! Ref T4712. I think we can close the book on T4712 with one more diff to clean up the array_reverse / reverse paging stuff? That diff is probably a bit tricky as it involes auditing every TransactionQuery callsite...
Test Plan: viewed a task with a lot of transactions. clicked "show older" and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10926
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: Updates header to use font-icons instead of images.
Test Plan: Test desktop and mobile layouts, Chrome, FF, Safari, IE.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10930
Summary: Only necessary for edits, only bother if the comment version is greater than 1. Ref T6690. This is another way to fix T6690 -- this check will never run since you can't edit a conpherence comment -- **but** the fix already applied should happen too to future proof Conpherence.
Test Plan: made a comment on a diff - success. edited the comment and mentions were generated.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10928
Summary: Fixes T6690. The editor innards end up loading the conpherence object, whose policy is dictated by these participation objects.
Test Plan: pre patch could not create new conpherences. post patch I can create conpherences! i can also add people to conpherences and it works.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10927
Summary:
Ref T4712. Specifically...
- Differential
- needed getApplicationTransactionViewObject() implemented
- Audit
- needed getApplicationTransactionViewObject() implemented
- Repository
- one object needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true)
- Ponder
- BONUS BUG FIX - leaving a comment on an answer had a bad redirect URI
- both PonderQuestion and PonderAnswer needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on both "history" controllers
- left a "TODO" on buildAnswers on the question view controller, which is non-standard and should be re-written eventually
- Phortune
- BONUS BUG FIX - fix new user "createNewAccount" code to not fatal
- PhortuneAccount, PhortuneMerchant, and PhortuneCart needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on Account view, merchant view, and cart view controller
- Fund
- Legalpad
- Nuance
- NuanceSource needed PhabricatorApplicationTransactionInterface implemented
- Releeph (this product is kind of a mess...)
- HACKQUEST - had to manually create an arcanist project to even be able to make a "product" and get started...!
- BONUS BUG FIX - make sure to "setName" on product edit
- ReleephProject (should be ReleepProduct...?), ReleephBranch, and ReleepRequest needed PhabricatorApplicationTransactionInterface implemented
- Harbormaster
- HarbormasterBuildable, HarbormasterBuild, HarbormasterBuildPlan, and HarbormasterBuildStep all needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) all over the place
Test Plan: foreach application, viewed the timeline(s) and made sure they still rendered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10925
Summary: Ref T4712. These are a bit time consuming to test so might as well send off a batch now and again.
Test Plan: foreach impacted controller, made sure the timeline rendered as it did before. for project column and config, noted the "should terminate" UI was also rendered unlike before.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10923
Summary: Fixes T6648. We do some automagical hotness based on the text you enter in remarkup textareas - e.g. adding projects or mentioning other objects. Refine the code here so that even when just editing a comment we build these transactions and apply them.
Test Plan: edited a comment and noted new mentions and projects showed up appropriately...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6648
Differential Revision: https://secure.phabricator.com/D10922
Summary:
Ref T4712. Thus far, it seems that most "non-standard" things can be done pretty easily in the controller. Aside from deploying, this diff had to fix a few bugs / missing implementations of stuff.
(Notably, PhabricatorAuthProviderConfig, HeraldRule, PhabricatorSlowvotePoll, and AlmanacNetwork needed to implement PhabricatorApplicationTransactionInterface, PhabricatorAuthAuthProviderPHIDType had to be added, and a rendering bug in transactions of type PhabricatorOAuth2AuthProvider had to be fixed.)
Test Plan: Almanac - looked at binding, device, network, and service view controllers and verified timeline displayed properly. Herald - looked at a rule and verified timeline. Slowvote - looked at a vote and verified timeline. Auth - looked at an auth provider (Facebook) and verified proper display of transactions within timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10921
Summary: These have all been modernized.
Test Plan: Browse Diffusion on a narrow screen.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10920
Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.
Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10887
Summary: Converts PHUIObjectItemView to use display: table rows and columns for more flexible layouts. Slightly increases spacing, improves mobile layouts. Fixes T5502
Test Plan: Tested in multiple applications and UIExamples. Ran through mobile, tablet, and desktop break points. Used IE8-IE10, Firefox, Chrome, Safari on both Mac and Windows.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5502
Differential Revision: https://secure.phabricator.com/D10917
Summary:
Fixes T6619. In `{Xnnn key=value, key=value}` we did not require a separator between the object and the key-value part. This could lead to `{rX11aaa}` being parsed as `{rX11 aaa}`, i.e. a reference to `rX11` with parameter `aaa` set.
Instead, require a space or comma before we'll parse key-value parts of embedded objects.
Test Plan:
Added and executed unit tests.
{F242002}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6619
Differential Revision: https://secure.phabricator.com/D10915
Summary: I think this was a "hacked" sub thing that never got updated when we switched to a real editor? I am not 100% sure how these methods are used, so please let me know if I should expand my test plan. Fixes T6659.
Test Plan: made a diff from the web ui, looked up the phid from mysql, ran bin/remove destroy <phid>, and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6659
Differential Revision: https://secure.phabricator.com/D10911
Summary: Fixes T6658.
Test Plan: made a diff with no repository and default policy and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6658
Differential Revision: https://secure.phabricator.com/D10910
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Fixes positioning issues by creating another container to hold the abs. positioned arrows. (Issues primarily presented on Workboards).
Test Plan: Test UI arrows on a workboard, applciation launcher, and in UIExamples.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10897
Summary:
Ref T6615. Mixing ASC and DESC ordering on a multipart key makes it dramatically less effective (or perhaps totally ineffective).
Reverse the meaning of the `priority` column so it goes in the same direction as the `id` column (both ascending, lower values execute sooner).
Test Plan:
- Queued 1.2M tasks with `bin/worker flood`.
- Processed ~1 task/second with `bin/phd debug taskmaster` before patch.
- Applied patch, took ~5 seconds for ~1.2M rows.
- Processed ~100-200 tasks/second with `bin/phd debug taskmaster` after patch.
- "Next in Queue" query on daemon page dropped from 1.5s to <1ms.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aklapper, 20after4, epriestley
Maniphest Tasks: T6615
Differential Revision: https://secure.phabricator.com/D10895
Summary: Ref T6615. Ref T3554. We need better tooling around the queue eventually, so start here.
Test Plan: Added 100K+ tasks locally with `bin/worker flood`. Executed some of them with `bin/phd debug taskmaster` (we already have a TestWorker, used in unit tests).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3554, T6615
Differential Revision: https://secure.phabricator.com/D10894
Summary: Helpful shortcut, I make quite a few at least
Test Plan: Click on dropdown, then Project. Get to Project Create page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10893
Summary: Fixes T6567, lets admins set a default policy for new Projects.
Test Plan: Changed the default Policy, created a new Project. Project had correct defaults.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6567
Differential Revision: https://secure.phabricator.com/D10890
Summary: **WIP** This adds default capability to the Files application
Test Plan: Set default to public, go to Files page, see public preset. Upload File. Doesn't work.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6564
Differential Revision: https://secure.phabricator.com/D10888
Summary:
Ref T6240. Some discussion in that task. In instance/cluster environments, daemons need to make Conduit calls that bypass policy checks.
We can't just let anyone add SSH keys with this capability to the web directly, because then an adminstrator could just add a key they own and start signing requests with it, bypassing policy checks.
Add a `bin/almanac trust-key --id <x>` workflow for trusting keys. Only trusted keys can sign requests.
Test Plan:
- Generated a user key.
- Generated a device key.
- Trusted a device key.
- Untrusted a device key.
- Hit the various errors on trust/untrust.
- Tried to edit a trusted key.
{F236010}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6240
Differential Revision: https://secure.phabricator.com/D10878
Summary: Fixes T4652, adding workboard link to emails
Test Plan: Move a task in a workboard from one column to another. Email notification should contain "WORKBOARD" section with link to that workboard
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4652
Differential Revision: https://secure.phabricator.com/D10889
Summary: For actions like "Close" that are in theory stopping the timeline, we should display some disruption to the line itself.
Test Plan:
Tested in UIExamples
{F236077}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10884
Summary: Ref T1217, Add link to email preferences to email template
Test Plan: Add comment to object like Maniphest task, check that email has a footer with a link to email preferences.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T1217
Differential Revision: https://secure.phabricator.com/D10883
Summary: Uses the check icon for closed, which is the primary action taken.
Test Plan:
Close as a duplicate, seen new icon.
{F236048}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10882
Summary: Finishes up work on PHUIInsetForm, cleans up the UI a bit when editing multiple tasks
Test Plan:
Edit multiple tasks, apply some new projects, still works as expected.
{F236044}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10881
Summary: Ref T5833. We try to load bindings even if we get no services back.
Test Plan: Queried invalid results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10880
Summary: Fixes T3189. Now if you say #projects in a commit message they will associate nicely with the commit. Also we record transactions about all this project editing fun.
Test Plan: tested migration by associating some projects with commits and verifying they still showed up post migration. tested adding / removing projects by doing so from the UI, noting transactions written nicely as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Projects: #projects
Maniphest Tasks: T3189
Differential Revision: https://secure.phabricator.com/D10877
Summary:
When a logged-out user views the home page, we currently compute a meaningless "assigned tasks" count which just counts every open task. Don't do this.
Ideally `ManiphestTaskQuery` should get some tightening up too (that is, `withOwners(array(null))` should not select every task), but that might affect other stuff and the performance implications of counting every open task are affecting WMF, so just fix the immediate issue for now.
Test Plan:
- Viewed homepage as a logged out user, no assigned task count.
- Viewed homepage as a logged-in user, still saw assigned task count.
Reviewers: btrahan, chad, chasemp
Reviewed By: chasemp
Subscribers: 20after4, epriestley
Differential Revision: https://secure.phabricator.com/D10876
Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.
Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6152
Differential Revision: https://secure.phabricator.com/D10875
Summary: Fixes T6200. Ref T6237. When creating a diff from the web view, allow the user to select the repository at that time. When viewing a diff that has no associated revision and then creating a revision, pass along the repository phid to the create revision controller. Within the create revision controller, default the repository selector to this repository phid. Finally, in the editor, stop aggressively resetting the repository phid for every TYPE_UPDATE; rather, do so if its not a new object -- the diff should reign supreme in that case -- or if there's no repository -- let the diff be the guide.
Test Plan:
- made a diff with an associated repo, made a revision from the diff, saw the associated repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo to nothing and it stuck on save!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6200
Differential Revision: https://secure.phabricator.com/D10872
Summary: Ref T6237. This sets us up for some future work like T6152, T6200 and generally cleaning up this workflow a bit. Tried to do as little as possible so not exposing transaction view yet. (Though that timeline is going to be a little funky in the common case of just the lone create transaction.)
Test Plan: made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237
Differential Revision: https://secure.phabricator.com/D10869
Summary: Ref T6343, adding HTMLMailMode to remarkup, and most objects should now be processed and appear pretty in emails.
Test Plan: Add a comment to a Maniphest task containing a mention of an object like '{T1}' or 'T1'. Emails should show a styled version of the object similar to how the object looks in the context of the Maniphest task in the UI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6343, T2617
Differential Revision: https://secure.phabricator.com/D10859
Summary: The shield is just confusing. In one case it doesn't work, and in the other case it just shows you a copy of the file you can see just below except in red. Fixes T4599, T1211. Note T1211 proposed not showing the "move away" file **at all** but I think removing the shield fixes the source of confusion. The code here is a bit if / else if / else if... heavy but this is logically sound.
Test Plan: made a diff where i moved a file then edited it in the new location. viewed diff, saw confusing shield, dropped caches, applied patch, viewed diff and saw no shield. made a diff where I moved a file and didn't edit in new location and saw similar shield disappearness.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1211, T4599
Differential Revision: https://secure.phabricator.com/D10865
Summary: Ref T6576. This avoids generating almost-empty HTML mail bodies for mail which incorrectly has no HTML body.
Test Plan: Generated some mail locally; the specific hook case is a pain for me to hit right now. Will push and dig in if that doesn't fix it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6576
Differential Revision: https://secure.phabricator.com/D10863
Summary:
Ref T4209. Depends on D10402.
This updates Conduit to support authenticating calls from other servers by signing the request parameters with the sending server's private key and verifying it with the public key stored in the database.
Test Plan:
- Made like 500 bad calls using the stuff in D10402.
- Made a few valid calls using the stuff in D10402.
Reviewers: hach-que, btrahan, #blessed_reviewers
Reviewed By: btrahan, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T6240, T4209
Differential Revision: https://secure.phabricator.com/D10401
Summary:
Ref T5833. Just building one query for now which returns the whole binding + interface + network + device tree. Maybe this will get split up in the future.
This will allow web hosts to call the central Almanac and pull instance configuration, authenticating with SSH.
Test Plan: {F234443}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10862
Summary: Fixes T6525, adds cc and tos to html emails
Test Plan: send html and plain emails, see new stuff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6525
Differential Revision: https://secure.phabricator.com/D10857
Summary: Fixes T6243.
Test Plan: simulated having this on by adding 'ONLY_FULL_GROUP_BY' to the $modes array. verified the error message looked nice {F233954}.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6243
Differential Revision: https://secure.phabricator.com/D10856
Summary: Fixes T4946. Theoretically.
Test Plan:
iiam
also unit tests.
also
```
cd /var/repo/X
git remote remove origin # simulates origin-missing clone under 1.7.1
cd /path/to/phabricator
./bin/repository pull X
```
and observed no errors
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4946, T5938
Differential Revision: https://secure.phabricator.com/D10855
Summary: This upgrades 1up view from "does not work" back to "barely works".
Test Plan: view diff, 1up and 2up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10854
Summary: Fixes T3046. Git has no minimum or bad versions; svn has no minimum and the bad version of 1.7.1, and mercurial has a minimum of 1.9 and bad versions 2.2 and 2.1. These error messages are specific about the problem but do not include the details as to what specifically is bad about a given version. I don't think that's really necessary - the details don't help solve the problem and its probably booooooring at best to the average user. The details about a bad version are included in the code however.
Test Plan: hardcoded a VCS array to let me test all the VCS stuff. added some phlog() calls to make sure the VCS version parsing stuff was working correctly relative to the version(s) on my system. played around with setting minimum versions and bad versions and saw good results depending on what I set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3046
Differential Revision: https://secure.phabricator.com/D10852
Summary:
Ref T6238. I'm building the instance management application now, but not putting it in the upstream -- I think the only use case for it is to build SAAS. If someone comes up with a use case (maybe a college course that wants to create an instance per-class or something?) we could open it up eventually, but it seems cleaner to keep it out of the upstream until we have such a use case.
I need to add schema patches. Make it easier for a subclass to just "add all the patches in this directory", like "autopatches/" works.
Test Plan:
- Ran `bin/storage status`, saw all normal patches still valid.
- In some future diff, the instances application will use this to apply patches.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238
Differential Revision: https://secure.phabricator.com/D10848
Summary: Fixes T6472. I was curious to see how many reports we'd get but we didn't get any I know of... Still, this is a nice convenient link.
Test Plan:
edited a wiki doc title + body. ran /bin/mail to see the body and liked what I saw
```TEXT BODY
btrahan changed the title from "new title is a good thing" to "new title is a good thing asdsadsadsadsada".
btrahan edited the document content.
DOCUMENT DIFF
http://phalanx.dev/phriction/diff/36/?l=3&r=4
DOCUMENT DETAIL
http://phalanx.dev/w/```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6472
Differential Revision: https://secure.phabricator.com/D10850
Summary: Fixes T1768. This is mostly a data cleanliness issue as duplicate rows don't really do anything, but let's clear it up now.
Test Plan: made some duplicate rows by adding the same auditor multiple times. ran ./bin/storage upgrade and it worked perfectly!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1768
Differential Revision: https://secure.phabricator.com/D10849
Summary: ...way way down in PhabricatorController. Use it on ManiphestTaskDetailController to test it. Ref T4712. I think the pager logic to be added as part of T4712 can safely reside entirely within this method. As I said earlier, 5 parameters is a lot, so I don't really want to add more. Next diff would do the pagination logic and the diff after that would deploy it everywhere. If while deploying it everywhere I find something off, that will be a different diff.
Test Plan: viewed maniphest tasks and they looked as spiffy as ever.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10844
Summary:
Fixes T6543. This was slightly trickier than I thought.
The actual inputs to this are: author, total affected count, added count, added list, removed count, removed list.
We weren't accounting for "total affected count" (used to select the correct word for "reviewers", e.g. "reviewers-few" vs "reviewers-many").
Test Plan: {F233357}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6543
Differential Revision: https://secure.phabricator.com/D10846
Summary: Fixes T5015, Allow Herald rules for Maniphest to act on task status changes.
Test Plan: Create Herald rule for Maniphest tasks to flag a task with status "wontfix". Change status of Maniphest task to "wontfix". Task should be flagged.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5015
Differential Revision: https://secure.phabricator.com/D10842
Summary: Fixes T6175. This particular option is only editable via the Applications application so let the user know that.
Test Plan: saw error message and clicked through to applications app. tried another locked option and saw old message
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6175
Differential Revision: https://secure.phabricator.com/D10841
Summary: ...if pertinent environment variables are set that is... Fixes T4151. This is the last piece in making repository creation somewhat easier.
Test Plan: made a new repo and noted that http serving was on r/w and ssh serving was still off, as expected for my environment configuration
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4151
Differential Revision: https://secure.phabricator.com/D10839
Summary:
Fixes T5492. I figured this would be easier to just fix than write a guide for; it actually took me an hour, but I spent like 75% of that futzing with my editor.
- The Move controller currently accepts either a slug or an ID. I can't find any callsites which pass a slug, and this doesn't make sense. Pretty sure this was copy/pasted from Edit or something. Only accept IDs.
- Slightly modernize the Move controller (newDialog(), handleRequest(), $viewer).
- When the user enters a bad slug, warn them that we're going to fix it for them and let them accept or reject the changes.
- Don't prefill the edit note (this feels inconsistent/unusual).
- On the form, label the input "Path" instead of "URI".
- Show the old path, to help remind the user what the input should look like.
- When a user tries to do a no-op move, show a more tailored message.
- When the user tries to do an overwriting move, explain how they can fix it.
- When normalizing a slug like `/question/???/mark/`, make it normalize to `/question/_/mark`.
Test Plan:
- Tried to move a document to itself.
- Tried to overwrite a document.
- Did a bad-path move, accepted corrected path.
- Did a good-path move.
- Did a path move with a weird component like `/???/`.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5492
Differential Revision: https://secure.phabricator.com/D10838
Summary:
- Order checks used `=` but intended `==`. We could probably write a lint rule for this.
- Selecting `*` with a join could pick (for example) `id` columns from both the document and content tables and end up using the wrong one.
- `%Q` expects a string and chokes on `null`.
Auditors: btrahan
Summary: Fixes T4666, add Herald rules to Phriction Documents
Test Plan: add Herald rule to flag if title contains "xyz", create Phriction Document with title "xyz". Phriction Document should be flagged.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4666
Differential Revision: https://secure.phabricator.com/D10830
Summary: When merging tasks, the corresponding transaction on the merged task should be black, and the transaction on the ultimate task should be green.
Test Plan: Create two tasks, merge one into the other, merged task transaction is black, the surviving task should show a green transaction.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6500
Differential Revision: https://secure.phabricator.com/D10827
Summary: when creating new documents the policy object wasn't being initialized properly. update the code to use the new handy initializeNewDocument method. Fixes T6527.
Test Plan: viewed a doc at /w/asdsadsadsdas/ and saw the correct policy setting
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6527
Differential Revision: https://secure.phabricator.com/D10837
Summary: i think way back in D10490 I didn't incorporate feedback correctly. make this code right as it fatals in this codepath as is. Fixes T6508.
Test Plan: @joshuaspence to the rescue (I remain unable to test this effectively with my baby-clean installation.)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, joshuaspence
Maniphest Tasks: T6508
Differential Revision: https://secure.phabricator.com/D10833
Summary: Fixes T6495. convert ad hoc query to a PhrictionDocumentQuery, thus enforcing view permissions
Test Plan: noted my test user a had a great wiki while test user b couldn't see most things.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6495
Differential Revision: https://secure.phabricator.com/D10822
Summary:
Ref T5833. I want to add SSH keys to Almanac devices, but the edit workflows for them are currently bound tightly to users.
Instead, decouple key management from users and the settings panel.
Test Plan:
- Uploaded, generated, edited and deleted SSH keys.
- Hit missing name, missing key, bad key format, duplicate key errors.
- Edited/generated/deleted/etc keys for a bot user as an administrator.
- Got HiSec'd on everything.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10824
Summary:
- The icon CSS tag is transformed through the new function
PhabricatorProjectIcon::getAPIName($key), which returns
a name without fa-.
- Color is a trivial lookup
- Profile image returns the PHID or null if not available
Test Plan:
- Create two projects, with different icon and color,
one with and one without profile image.
- Request information on both using project.query
Then:
[ ] Confirm icon and colors are correct for both projects
[ ] Confirm image PHID is correct
[ ] Confirm image PHID is null for the project without image
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: yuvipanda, Korvin, legoktm, epriestley
Maniphest Tasks: T6501
Differential Revision: https://secure.phabricator.com/D10823
Summary: ...how do you lock down entire areas otherwise? Fixes T6496.
Test Plan: used user 1 to create x/y that user 2 can't edit. tried to create x/y/z as user 2 and got a big ole error dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6496
Differential Revision: https://secure.phabricator.com/D10819
Summary: 'cuz the wiki don't play that no more. Fixes T6497. This is mainly important to fix an incorrect policy filtering issue where a project policy can incorrectly override a document policy. Otherwise, it makes things nice and clean.
Test Plan:
- viewed the wiki - success.
- viewed wiki document list under "index" and tried a few different queries
- grep'd for things like "hasProject" and "getProject" and saw no phriction-related results
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6497
Differential Revision: https://secure.phabricator.com/D10818
Summary: Allow `./bin/diviner generate` to continue even if there is an exception throw processing an atom. This allows Diviner documentation to be generated for PHP source code that cannot be parsed with XHPAST.
Test Plan: Ran `./bin/diviner generate` on a PHP repository which previously throw an `XHPASTSyntaxErrorException`.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10803
Summary: Fixes T5387. I broke this in D8805. Fix it by using the application search parameters that D8805 introduced.
Test Plan: verified that the two links mentioned in T5387 worked for me. Also tried manual links on secure.phabricator.com and those showed the right data even...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5387
Differential Revision: https://secure.phabricator.com/D10817
Summary:
Ref T4029. Fixes T6034.
Various front-end miscellania here. See D10814#96251. This more or less makes policy work but I am not going to call it "fixed" here since we need D10814 to be deployed too and will do that manually.
Test Plan:
- changed document policy from web ui and changes persisted
- changed document policy from web and had form error and changes persisted
- created a structure like users/users/justmyuserpolicy and made sure another user could delete the users/users/ doc
- moved a doc from a to b and verified policy persisted
- verified stub documents inherited policy of the document that stub them...!
- uploaded a file and verified that it 1) had the permissions of the page it was added to and 2) had an "attached" tab linking back to the page on the file page (this means T6034 is fixed with this)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6034, T4029
Differential Revision: https://secure.phabricator.com/D10816
Summary: Ref T4029. this diff makes the pertinent database changes AND adds the migration script. This is important to get the data backend straightened away before we fully ship T4029. Next diff will expose the edit controls for these policies and whatever else work is needed to get that part done right.
Test Plan: made sure the lone project page on my wiki had a project with restrictive view policy. Post migration verified correct policy applied to this lone project page AND most open policy applied to the others
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10814
Summary:
Ref T5833. This fixes a few weird things with this table:
- A bunch of columns were nullable for no reason.
- We stored an MD5 hash of the key (unusual) but never used it and callers were responsible for manually populating it.
- We didn't perform known-key-text lookups by using an index.
Test Plan:
- Ran migrations.
- Faked duplicate keys, saw them clean up correctly.
- Added new keys.
- Generated new keys.
- Used `bin/auth-ssh` and `bin/auth-ssh-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10805
Summary:
Fixes T1191. I'll write up the changelog with notes about this and open a feedback task for followups.
When you run `storage upgrade`, automatically run `storage adjust` afterward. Provide a flag to disable this.
This brings everyone into the utf8mb4 world.
Test Plan: Ran `bin/storage upgrade` with various flags. Ran `bin/storage adjust`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10800
Summary: Missed this in previous pass. Send these as links in HTML emails.
Test Plan: Register a new user that nees approval.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10815
Summary:
Ref T1191. Use `storage quickstart` to regenerate `quickstart.sql` using modern schema construction statements.
This puts new installs into utf8mb4 mode immediately without requiring storage adjustment.
Test Plan:
- Ran `arc unit --everything`, which uses quickstart.
- Ran `bin/storage upgrade --namespace temp`, to quickstart a new namespace.
- Ran `bin/storage upgrade --namespace temp --disable-utf8mb4`, to quickstart a new namespace without utf8mb4 support.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10797
Summary: @btrahan asked about this but I gave the wrong answer. These
currently do not turn into links. I think they might have in the past,
but if they did the rule is a little weird and feels specific to my
use. We can reexamine this at some point, but for now just make the
links work in a normal, reasonable sort of way.
Auditors: btrahan
Summary: Fixes T6262. Ref T4029. Also gets us ready for T5873 for these end points. I can file something new about someday adding phriction.query, etc but I think we'll remember and can look at that post T5873.
Test Plan: made a document via conduit and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6262, T4029
Differential Revision: https://secure.phabricator.com/D10813
Summary: Ref T4029. Even more code consolidation and cleanup for the long term benefits!
Test Plan: moved a page successfully. tried to move a page to an existing page and got an error. did the two tab trick to try to move a deleted page and got an error.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10812
Summary:
- Warn users that they'll need to be comfortable with the CLI.
- Move XHProf stuff to the developer docs, since few/no normal users need it.
Test Plan: Read documentation.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10810
Summary:
- Direct users to detailed bug report / feature reuqest documents.
- Move "get more info" and "unreproducible problems" to bug reporting document.
- Stop telling users to email us, and strongly encourage them to use primary channels.
Test Plan: Read documentation.
Reviewers: btrahan, chad
Reviewed By: btrahan, chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10808
Summary: Ref T4029. More cleanup and code consolidation for the long terms benefits.
Test Plan: found a document and opened up two browser tabs. Loaded delete dialog on both. Completed delete in one tab and noted document was properly deleted. Tried to complete delete in tab 2 and got an error message.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10809
Summary: Ref T1191. Same deal as D10786. These were previously case-insensitive, but changed to a case-sensitive column type.
Test Plan: Ran `bin/storage adjust` and got and adjustment.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: webframp, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10806
Summary: Fixes T6489. In an earlier diff I forgot to update the document status itself, thinking that the content update percolated up magically. Restore this functionality so the wiki works better.
Test Plan: deleted a document and observed that i did not get the option to delete it again and it disappeared from document index
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6489
Differential Revision: https://secure.phabricator.com/D10807
Summary:
Ref T4029. Some business logic lives outside the editor. This revision moves that logic from the edit controller into the editor proper. This makes re-using that business logic across other endpoints - say like a conduit end point - possible. This is also part of the general modernization quest for phriction I am on.
This diff also restores the functionality where you can delete a document by wiping out the content and saving.
Test Plan: tried to make a document with no title or content and saw errors. opened a document for edit with user 1, then made edits with user 2, then saw an error when i made the edit with user 1. clicking "overwrite changes" then worked. deleted a document by wiping out the body and clicking save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10795
Summary:
Ref T4029. Long live PhrictionTransactionEditor...! this means that all existing functionality runs 100% through the modern transactions + editor framework. this diff does a few things in sum
- kills the old editor
- moves conduit-based edits to new editor
- moves stubbing out documents to new editor
- deletes moving of wiki docs for projects functionality... (T4021#59511 is a better bigger battle plan here.)
Test Plan: edited a phriction document via conduit and it worked. created a new phriction document /that/was/deep/ and verified ancestral documents were properly stubbed out. changed a project name and noted no wiki page moves.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10792
Summary: Ref T5833. Since these will no longer be bound specifically to users, bring them to a more central location.
Test Plan:
- Edited SSH keys.
- Ran `bin/ssh-auth` and `bin/ssh-auth-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10791
Summary:
Ref T5833. Currently, SSH keys are associated only with users, and are a bit un-modern. I want to let Almanac Devices have SSH keys so devices in a cluster can identify to one another.
For example, with hosted installs, initialization will go something like this:
- A request comes in for `company.phacility.com`.
- A SiteSource (from D10787) makes a Conduit call to Almanac on the master install to check if `company` is a valid install and pull config if it is.
- This call can be signed with an SSH key which identifies a trusted Almanac Device.
In the cluster case, a web host can make an authenticated call to a repository host with similar key signing.
To move toward this, put a proper Query class on top of SSH key access (this diff). In following diffs, I'll:
- Rename `userPHID` to `objectPHID`.
- Move this to the `auth` database.
- Provide UI for device/key association.
An alternative approach would be to build some kind of special token layer in Conduit, but I think that would be a lot harder to manage in the hosting case. This gives us a more direct attack on trusting requests from machines and recognizing machines as first (well, sort of second-class) actors without needing things like fake user accounts.
Test Plan:
- Added and removed SSH keys.
- Added and removed SSH keys from a bot account.
- Tried to edit an unonwned SSH key (denied).
- Ran `bin/ssh-auth`, got sensible output.
- Ran `bin/ssh-auth-key`, got sensible output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10790
Summary: Ref T4214, this breaks the 'eye' out as a separate image 40px x 40px. We also now show the eye on mobile, as we have enough room for both currently.
Test Plan: Tested default and nightmaremoon colors, tested mobile, tablet and desktop layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4214
Differential Revision: https://secure.phabricator.com/D10794
Summary: Fixes T6480. @epriestley is very popular and has over 100 messages, thus triggering this issue. fix the typo because bugs are bad.
Test Plan: set limit to 1, observed fatal, applied patch and fatal went away
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6480
Differential Revision: https://secure.phabricator.com/D10793
Summary:
Fixes T2792. This adds a pluggable configuration layer between all the stuff on disk (local/file) and the runtime configurable stuff (database).
An install can subclass this source and:
- For Phacility, query a remote service (like Almanac) to retrieve hostname-based configuration, allowing one install to serve multiple instances.
- Maybe for Phacility, query a remote service (like Phlux) to retrieve sitevar-like configuration (e.g., put everything in readonly mode to deal with a maintenance issue?). Not sure if we'll do this or not. We might just nuke Phlux since Almanac is sort-of-a-superset of it for our purposes.
- For third parties, query some other remote service if that makes config management easier. In particular, it would theoretically let you put locked config in Zookeeper or whatever else you want.
Test Plan: Added a fake source and saw it inject configuration.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2792
Differential Revision: https://secure.phabricator.com/D10787
Summary:
Ref T5833. Allow services and devices to be tagged with projects.
(These fluff apply implementations are a good example of the issue discussed in T6403.)
Test Plan: {F229569}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10782
Summary:
Ref T5833. Adds support for arbitrary properites to Almanac devices and bindings.
- For Devices, this allows you to maybe mark what `rack` a server is on, the `serial` number of a router, etc.
- For Bindings, this allows you to maybe mark that a bound device is `active`, provide `credentials`, expose it as `readonly`, etc.
Test Plan: Added properties to Devices and Bindings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10781
