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. 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: 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:
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: 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: 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 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:
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: 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:
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:
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 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 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 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 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: 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: 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 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: 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: 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 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: 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: 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:
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:
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:
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: 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:
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: 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: 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. 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 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 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: 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 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: 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:
...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:
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 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: 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 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:
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: 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:
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: 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: 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: 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:
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 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 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:
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:
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:
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 T1191. Currently if a developer forgot to specify a column type, `storage adjust` aborts explosively mid-stream. Instead:
- Make this a formal error with an unambiugous name/description instead of something you sort of infer by seeing "<unknown>".
- Make this error prevent generation of adjustment warnings, so we don't try to `ALTER TABLE t CHANGE COLUMN c <unknown>`, which is nonsense.
- When schemata errors exist, surface them prominiently in `storage adjust`.
Overall:
- Once `storage upgrade` runs `storage adjust` automatically (soon), this will make it relatively difficult to miss these errors.
- Letting these errors slip through no longer escalates into a more severe issue.
Test Plan:
Commented out the recent `mailKey` spec and ran `storage adjust`:
```
$ ./bin/storage adjust --force
Verifying database schemata...
Found no adjustments for schemata.
Target Error
phabricator2_phriction.phriction_document.mailKey Column Has No Specification
SCHEMATA ERRORS
The schemata have serious errors (detailed above) which the adjustment
workflow can not fix.
If you are not developing Phabricator itself, report this issue to the
upstream.
If you are developing Phabricator, these errors usually indicate that your
schema specifications do not agree with the schemata your code actually
builds.
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10771
Summary:
Fixes an issue with T5336 / D9871. We did 99% of the work here but didn't actually turn on the priority sorting. The unit test passed by default, which didn't catch this.
- Fix the unit test (it failed).
- Fix the query (test now passes).
- Add a "Next in Queue" element to the UI to make this kind of thing easier to spot/understand.
Test Plan: Ran unit test. Viewed "Next in Queue". Queued some tasks, flushed the queue. Web UI tracked the state sensibly.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Differential Revision: https://secure.phabricator.com/D10766
Summary:
Ref T1191. Notable stuff:
- Adds `--disable-utf8mb4` to `bin/storage` to make it easier to test what things will (approximately) do on old MySQL. This isn't 100% perfect but should catch all the major stuff. It basically makes us pretend the server is an old server.
- Require utf8mb4 to dump a quickstart.
- Fix some issues with quickstart generation, notably special casing the FULLTEXT handling.
- Add an `--unsafe` flag to `bin/storage adjust` to let it truncate data to fix schemata.
- Fix some old patches which don't work if the default table charset is utf8mb4.
Test Plan:
- Dumped a quickstart.
- Loaded the quickstart with utf8mb4.
- Loaded the quickstart with `--disable-utf8mb4` (verified that we get binary columns, etc).
- Adjusted schema with `--disable-utf8mb4` (got a long adjustment with binary columns, some truncation stuff with weird edge case test data).
- Adjusted schema with `--disable-utf8mb4 --unsafe` (got truncations and clean adjust).
- Adjusted schema back without `--disable-utf8mb4` (got a long adjustment with utf8mb4 columns, some invalid data on truncated utf8).
- Adjusted schema without `--disable-utf8mb4`, but with `--unsafe` (got truncations on the invalid data).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10757
Summary:
Fixes T6416. The comment is consistent with intent, but the actual regexp doesn't quite work right. In particular, we incorrectly match `#security.` as `security.` (with a period) instead of `security` (with no period).
Since this stuff is a pain to test and I evidently got it wrong in this case in D8703, make it unit testable.
Test Plan:
Added unit tests. Also:
{F227181}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6416
Differential Revision: https://secure.phabricator.com/D10753
Summary: Fixes T6399. This allows you to use global search to find projects by searching for text in their descriptions.
Test Plan: Added a unique word to a project description, reindexed it, searched, got a hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6399
Differential Revision: https://secure.phabricator.com/D10748
Summary: Ref T5702. This primarily gets URI routing out of Aphront and into an Application, for consistency.
Test Plan: Loaded some pages, got static resources.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10696
Summary:
Ref T2787.
- Account members can add and remove other members (major use case is corporate accounts).
- Use a modern edge constant setup.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10678
Summary: Fixes T6265, allows you to pass required:false as a parameter.
Test Plan: Add required:false to a field, no longer see "Required"
Reviewers: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6265
Differential Revision: https://secure.phabricator.com/D10659
Summary:
Ref T2787. These were still stuck in the stone ages.
(The handles are pretty skeletal but most aren't used anywehre.)
Test Plan: Funded an initiative without anything breaking. Grepped for removed constants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10647
Summary: without escapage here, creating databases fails. Fixes T6251.
Test Plan: ran the command CREATE DATABASE foo COLLATION binary and it failed; ran the command CREATE DATABASE foo2 COLLATION "binary" and it worked; trusting that the %T still works as advertised.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6251
Differential Revision: https://secure.phabricator.com/D10641
Summary:
Ref T2787. Phortune currently stores a bunch of stuff as `...inUSDCents`. This ends up being pretty cumbersome and I worry it will create a huge headache down the road (and possibly not that far off if we do Coinbase/Bitcoin soon). Even now, it's more of a pain than I figured it would be.
Instead:
- Provide an application-level serialization mechanism.
- Provide currency serialization.
- Store currency in an abstract way (currently, as "1.23 USD") that can handle currencies in the future.
- Change all `...inUSDCents` to `..asCurrency`.
- This generally simplifies all the application code.
- Also remove some columns which don't make sense or don't make sense anymore. Notably, `Product` is going to get more abstract and mostly be provided by applications.
Test Plan:
- Created a new product.
- Purchased a product.
- Backed an initiative.
- Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10633
Summary: Ref T1191. We don't create new databases with appropriate collation yet.
Test Plan:
Created a new database and saw it issue:
```
>>> [10] <query> CREATE DATABASE IF NOT EXISTS `phabricator2_testo` COLLATE utf8mb4_bin
```
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10632
Summary:
Ref T4209. This creates storage for public keys against authorized hosts, such that servers can be authorized to make Conduit calls as the omnipotent user.
Servers are registered into this system by running the following command once:
```
bin/almanac register
```
NOTE: This doesn't implement authorization between servers, just the storage of public keys.
Placing this against Almanac seemed like the most sensible place, since I'm imagining in future that the `register` command will accept more information (like the hostname of the server so it can be found in the service directory).
Test Plan: Ran `bin/almanac register` and saw the host (and public key information) appear in the database.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D10400
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T6223. Two issues:
- We don't use `/u` mode on these regexps. Without `/u`, the `\w`/`\W`/`\s`/`\S` modifiers have bad behavior on non-ASCII bytes. Add the flag to use unicode mode, making `\w` and `\s` behave like we expect.
- We might possibly want to do something different here eventually (for example, if the `/u` flag has some huge performance penalty) but this seems OK for now.
- We use `\b` (word boundary) to terminate the match, but `🐳` is not a word character. Use `(?!\w)` instead ("don't match before a word character") which is what we mean.
Test Plan: {F211498}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6223
Differential Revision: https://secure.phabricator.com/D10618
Summary: Ref T1191. The bulk of the slowness in T1191 is copying tables. In some cases, we can't avoid this, but we have various readthrough caches which may be very large and are safe to drop, and dropping them is very quick (much less than 1 second). In particular, dropping the `changeset_parse_cache` made the process at least ~8 minutes faster on `secure.phabricator.com` (I killed it after 8 minutes, so I'm not sure what the real number is).
Test Plan: Ran `bin/storage adjust` and saw it drop caches before applying adjustments.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10616
Summary: Ref T1191. I renamed the phases but missed these two since I didn't have any more key issues locally.
Test Plan: Ran `bin/storage adjust` in production with key issues.
Reviewers: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10612
Summary:
Ref T1191. When changing the column type of an AUTO_INCREMENT column, we currently may lose the autoincrement attribute.
Instead, support it. This is a bit messy because AUTO_INCREMENT columns interact with PRIMARY KEY columns (tables may only have one AUTO_INCREMENT column, and it must be a primary key). We need to migrate in more phases to avoid this issue.
Introduce new `auto` and `auto64` types to represent autoincrement IDs.
Test Plan:
- Saw autoincrement show up correctly in web UI.
- Fixed an autoincrement issue on the XHProf storage table with `bin/storage adjust` safely.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10607
Summary:
Ref T1191. Currently, the `quickstart.sql` gets generated in a pretty manual fashion. This is a pain, and will become more of a pain in the world of utf8mb4.
Provide a workflow which does upgrade + adjust + dump + destroy, then massages the output to produce a workable `quickstart.sql`.
Test Plan: Inspected output; I'll test this more throughly before actually generating a new quickstart, but that's some ways away.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10603
Summary:
Ref T1191. For most text columns, we either don't care if "a" and "A" are the same, or we expect them to be different (for example: keys, domains, secrets, etc). Default text columns to the `_bin` collation so they are compared by strict character value. This is safer in cases where we aren't sure.
For some text columns, we allow the user to sort by the column in the UI (like Maniphest task titles) or we do care that "A" and "a" are the same (for example: project names). Introduce a new class of virtual data types, the "sort..." types, to cover these columns. These are like the "text..." types but use sorting collations which treat "A" and "a" the same.
Test Plan:
- Made an effort to identify all columns where the UI relies on database collation.
- Ran `bin/storage adjust` and cleared all warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: beng, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10602
Summary:
Ref T1191. These are a bit tricky because keys can interact with column changes, so basically we do three phases:
1. Nuke all bad keys.
2. Make all column (and database/table) changes.
3. Fix all nuked keys.
Test Plan: Ran migration locally. See note for remaining issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10599
Summary:
Ref T1191. Adds a new workflow which can apply schema adjustments.
For now, it only performs database and table collation/charset adjustments. I believe these are extremely safe/minor, because they only affect the default values for newly created columns.
Test Plan:
- Ran migration on various database states, database/table changes went through cleanly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10595
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary:
Ref T1191. Nothing too notable here:
- Allow a Lisk object to specify that there's no expectation that a table exists. We have one Harbormaster object and one Token object like this.
- Removed BuildPlanTransactionComment because it's currently unused.
Test Plan:
- Saw ~200 fewer warnings; just ~800 left.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10583
Summary:
Ref T1191. Some notes here:
- Drops the old LDAP and OAuth info tables. These were migrated to the ExternalAccount table a very long time ago.
- Separates surplus/missing keys from other types of surplus/missing things. In the long run, my plan is to have only two notice levels:
- Error: something we can't fix (missing database, table, or column; overlong key).
- Warning: something we can fix (surplus anything, missing key, bad column type, bad key columns, bad uniqueness, bad collation or charset).
- For now, retaining three levels is helpful in generating all the expected scheamta.
Test Plan:
- Saw ~200 issues resolve, leaving ~1,300.
- Grepped for removed tables.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10580
Summary: See <https://github.com/phacility/phabricator/issues/665>. From reading documentation, this seems dramatically better for InnoDB tables than the default behavior.
Test Plan: Ran `bin/storage dump`, got a reasonable-looking dump.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10606
Summary: Ref T1191. Handful of minor things here (T6150, T6149, T6148, T6147, T6146) but nothing very noteworthy.
Test Plan: Viewed web UI, saw fewer errors.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10527
Summary: Fixes T6177. Now that we've reframed "Beta" into "Prototype", there's no reason this needs to be in a separate super-hidden class of application anymore.
Test Plan: Saw Releeph available as a normal Prototype application.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6177
Differential Revision: https://secure.phabricator.com/D10550
Summary: Fixes T5374. Add an acceptance test to the `PhabricatorInfrastructureTestCase` class which fails if a Celerity map is not up-to-date. In order to achieve this, a lot of code used to generate Celerity maps was transferred from `CelerityManagementMapWorkflow` to `CelerityResourceMap` and `CelerityResourceMapGenerator`.
Test Plan: Ran `arc unit` and noticed that all tests passed. Modified a JavaScript file and ran `arc unit` again (without running `./bin/celerity map`)... this time the test failed, as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5374
Differential Revision: https://secure.phabricator.com/D9817
Summary:
Ref T1191.
- Adds support for custom fields.
- Adds support for partial indexes (indexes on a prefix of a column).
- Drops old auxiliary storage table: this was moved to custom field storage about a year ago.
- Drops old project table: this was moved to edges about two months ago.
Test Plan:
- Viewed web UI, saw fewer issues.
- Used `grep` to verify no readers/writers for storage or project table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10526
Summary:
Ref T1191. Three parts:
- The old way of getting key information only got primary / unique / foreign keys, not all keys. Use `SHOW INDEXES` to get all keys instead.
- Track key uniqueness and raise warnings about it.
- Add a new "all issues" view to show an expanded, flat view of all issues. This is just an easier way to get a list so you don't have to dig around in the hierarchical view.
Test Plan:
{F206351}
{F206352}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10525
Summary: T1191. Nothing very notable here.
Test Plan: Saw more blue in web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10522
Summary: Ref T1191. Nothing too exciting in these.
Test Plan: Saw more blue in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10521
Summary:
Ref T1191. Notable:
- Allowed objects to remove default columns (some feed tables have no `id`).
- Added a "note" severity and moved all the charset stuff down to that to make progress more clear.
Test Plan:
Trying to make the whole thing blue...
{F205970}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10519
Summary: Ref T1191. Fills in some more of the databases. Nothing very notable here. I didn't encounter any issues or overlong keys.
Test Plan: Used web UI to click around and verify expected schemata match up against actual schemata well.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10516
Summary: Ref T1191. This fills in some more features and gets audit and auth nearly generating reasonable expected schemata.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10500
Summary:
Ref T1191. This lays some groundwork for generating the expected schemata, so we can compare them to the actual schemata and produce a meaningful diff.
- In general, each application will subclass `PhabricatorConfigSchemaSpec` and provide a definition of the tables it expects.
- This class has helper methods to mostly-automatically build table definitions for Lisk and (in the future) edges.
- When building expected schema, we specify a "data type", like "epoch". This is the type of data the application stores in the column, from the application's point of view. The SchemaSpec converts this into the best avilable storage type: for example, "text" will translate to `utf8mb4` if it's availalbe, or `binary` if not. This gives us a layer of indirection to insulate us from craziness.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10497
Summary:
Ref T1191. Plan here is:
- Build a tool showing the current schemata status (this diff).
- Have it compare the current status to the desired status (partly here, mostly in future diffs).
- Then add a migration tool, and eventually a setup issue to tell people to run it.
Test Plan:
Reviewed current schemata.
{F204492}
{F204493}
{F204494}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10494
Summary:
Fixes T6084. Changes:
- Rename `phabricator.show-beta-applications` to `phabricator.show-prototypes`, to reinforce that these include early-development applications.
- Migrate the config setting.
- Add an explicit "no support" banner to the config page.
- Rename "Beta" to "Prototype" in the UI.
- Use "bomb" icon instead of "half star" icon.
- Document prototype applications in more detail.
- Explicitly document that we do not support these applications.
Test Plan:
- Ran migration.
- Resolved "obsolete config" issue.
- Viewed config setting.
- Browsed prototypes in Applications app.
- Viewed documentation.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6084
Differential Revision: https://secure.phabricator.com/D10493
Summary:
Ref T5835. This is all pretty boilerplate, and does not interact with Phortune at all yet.
You can create "Initiatives", which have a title and description, and support most of the expected infrastructure (policies, transactions, mentions, edges, appsearch, remakrup, etc).
Only notable decisions:
- Initiatives have an explicit owner. I think it's good to have a single clearly-responsible user behind an initiative.
- I think that's it?
Test Plan:
- Created an initiative.
- Edited an initiative.
- Changed application policy defaults.
- Searched for initiatives.
- Subscribed to an initiative.
- Opened/closed an initiative.
- Used `I123` and `{I123}` in remarkup.
- Destroyed an initiative.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10481
Summary: see title. Ref T5875.
Test Plan: Merged one task into another task - verified transactions on both tasks. Merged two tasks into another task - verified transactions on all three tasks. Checked out my feed and saw MERGE_INTO stories and MERGE_FROM stories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5875
Differential Revision: https://secure.phabricator.com/D10427
Summary: Shows the UI everywhere. Also asort() the keys before calculating the environment hash as that is probably an issue for someone at some point we just don't need to have. Ref T5968.
Test Plan: Viewed the setup check and saw a link to the daemon console. Viewed the daemon console and saw the various stale config daemons. Clicked a daemon and saw a "stale config" header icon where expected. Restarted daemons and all of this went away.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5968
Differential Revision: https://secure.phabricator.com/D10367
Summary:
Fixes T4057. This sort of sidesteps the trickiest (but very rare) case of things like embedded slowvotes. We might be able to refine that later.
In the common bad case (macros, large images) it gets reasonable results by using `overflow: hidden` with `max-height`.
We use `PhabriatorMarkupEngine::summarize()` to try to just render the first paragraph.
Test Plan: {F195093}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4057
Differential Revision: https://secure.phabricator.com/D10355
Summary: Fixes T4769. This is silly and just scratches an itch, but do a better job with navigation sequences.
Test Plan: {F195082}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4769
Differential Revision: https://secure.phabricator.com/D10353
Summary: Fixes T2605. Provide some instructions on configuring RDS properly. The "DB Parameter Group" thing in the web UI seems pretty easy to use, it's just not obvious that it's what you should be using.
Test Plan: Jiggled these warnings to trigger them, viewed the output, saw a table of values and a hint about RDS.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2605
Differential Revision: https://secure.phabricator.com/D10343
Summary: Fixes T4881.
Test Plan: made a config change, saw the issue, restarted daemons and it went away
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4881
Differential Revision: https://secure.phabricator.com/D10339
Summary:
Fixes T5916. Key insight here is that the screenshot shows a custom "Detail Solution / Notes" field, which is why this mojo doesn't work: custom remarkup fields don't emit their content for mention/file extraction.
Also fix a bug where multiple blocks with file PHIDs could be merged improperly, discarding some file PHIDs.
Test Plan: Added a custom remarkup field, added files to it, saw them attach to the task when changes were saved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5916
Differential Revision: https://secure.phabricator.com/D10335
Summary:
Fixes T5934. If you hash a password with, e.g., bcrypt, and then lose the bcrypt hasher for some reason, we currently fatal when trying to figure out if we can upgrade.
Instead, detect that the current hasher implementation has vanished and let the user reset their password (for account passwords) or choose a new one (for VCS passwords)>
Test Plan:
Account password:
- Artifically disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Used password reset workflow to change password, saw iterated MD5 hashed password get set.
- Enabled bcrypt hasher again.
- Saw upgrade warning.
- Upgraded password to bcrypt.
VCS password:
- Artificially disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Reset password.
- Saw iterated md5 password.
- Reenabled bcrypt.
- Upgraded to bcrypt.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5934
Differential Revision: https://secure.phabricator.com/D10325
Summary:
Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com".
- Reject local URIs with backslashes (we never generate these).
- Fully-qualify all "Location:" redirects.
- Require external redirects to be marked explicitly.
Test Plan:
- Expanded existing test coverage.
- Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction).
- Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected).
- Went through some local redirects (login, save-an-object).
- Verified file still work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10291
Summary:
Ref T5885. See D10276.
Currently, ActionHeaders can only have minicons, and we don't use them anywhere and they probably don't make much sense in the product anymore.
Instead, allow them to have font icons. Remove minicons, which have no callsites and probably won't in the future.
Test Plan:
{F190925}
- Grepped for `minicons`.
- Grepped for `setHeaderIcon()`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10277
Summary: Fixes T5884. Macro images are no longer public on most installs. We could generate tokens for them, but this (using Conduit to pull the file data) is easier and more correct.
Test Plan: Logged a bot into IRC and had it spam part of a macro before being killed for flooding.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10274
Summary: Fixes T5453.
Test Plan: made a remarkup comment that "Q1 is dumb and Q10 is awesome" and only Q10 was linked. changed the new setting to have the value " " and the Q1 also started linking.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5453
Differential Revision: https://secure.phabricator.com/D10270
Summary: Fixes T5883. The first time we hit an error we'll continue forward; we only bail after the second time. Instead, check for an error immediately
Test Plan: HA HA HA DID NOT TEST HA HA HA HA
Reviewers: btrahan, cburroughs
Reviewed By: cburroughs
Subscribers: epriestley
Maniphest Tasks: T5883
Differential Revision: https://secure.phabricator.com/D10265
Summary: Fixes T3173. This doesn't actually fix T3173 but I'm going to redirect that. It does make the bot quit IRC gracefully, with a nicer quit message, which can be customized.
Test Plan: Got a bot to quit IRC with nice messages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3173
Differential Revision: https://secure.phabricator.com/D10257
Summary: Fixes T2101. When viewing an image change, show image dimensions, MIME type, and filesize.
Test Plan:
{F190189}
{F190190}
very utility
such wow
Reviewers: mailson, btrahan, chad
Reviewed By: chad
Subscribers: epriestley, Korvin, aran
Maniphest Tasks: T2101
Differential Revision: https://secure.phabricator.com/D5206
Summary:
Fixes T5855. Adds a `--graceful N` flag to `phd stop` and `phd restart`.
`phd` will send SIGINT, wait `N` seconds, SIGTERM, wait 15 seconds, and SIGKILL. By default, `N` is 15.
Test Plan:
- Ran `bin/phd debug ...` and used `^C` to interrupt daemons. Saw graceful shutdown behavior, and abrupt termination on multiple `^C`.
- Ran `bin/phd start`, `bin/phd stop` and `bin/phd restart` with `--graceful` set to various things, notably `0`. Saw graceful shutdowns on the CLI and in the web UI. With `0`, abrupt shutdowns.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5855
Differential Revision: https://secure.phabricator.com/D10228
Summary:
Fixes T5837. The problem is that the hash is being recognized as a commit hash. We currently fire the object monogram rules fairly early, but there's no real reason to do this. Move them after all of the hyperlink rules:
0 PhutilRemarkupEscapeRemarkupRule
100 PhutilRemarkupMonospaceRule
150 PhutilRemarkupDocumentLinkRule
175 PhrictionRemarkupRule
<<< OLD OBJECT RULE POSITION
200 PhabricatorIconRemarkupRule
200 PhabricatorMemeRemarkupRule
200 DivinerSymbolRemarkupRule
350 DoorkeeperRemarkupRuleJIRA
350 PhabricatorYoutubeRemarkupRule
350 DoorkeeperRemarkupRuleAsana
400 PhutilRemarkupHyperlinkRule
>>> NEW OBJECT RULE POSITION
500 PhabricatorImageMacroRemarkupRule
500 CustomInlineJIRA5Rule
500 PhabricatorMentionRemarkupRule
500 CustomInlineCodeRule
1000 PhutilRemarkupDelRule
1000 PhutilRemarkupBoldRule
1000 PhutilRemarkupItalicRule
1000 PhutilRemarkupUnderlineRule
- The disadvantage of this approach is that `{F123, alt=go look at http://lol.com/ omg}` will parse the URL first, and then fail to resolve the object embed. This seems very rare / unusual.
- The advantage is that all URLs which happen to have monograms in them work.
In the future, we could refine this by separating the rules, so the embed (`{...}`) versions fired at priority 200, while the normal versions fired at priority 450. We can wait for use cases, though. This is a little messy because the same code implements both rules.
Test Plan:
- Verified example in T5837.
- Marked up object rules like `F123` (works), `[[ asdf | F123 ]]` (works), `{F123, alt=http://example.com}` (does not work).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5837
Differential Revision: https://secure.phabricator.com/D10212
Summary:
See some discussion here:
24a6eeb8d8 (commitcomment-7334892)
The `protected $properties;` storage parameter added to `ProjectColumn` is shadowed by `getProperties()` in the base class.
Although this works correctly for me, it's ambiguous and worth fixing. Make the base class methods explicit.
Test Plan: Used `grep` to find callers for both methods and renamed them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10210
Summary: Just wanted to play with this, removes the gradient 'cards' for a flat design.
Test Plan:
Tested various apps, workboards
{F166127}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9515
Summary:
Currently, we'll try to publish notifications while running tests. This is at best unnecessary and at worst problematic (we don't stub out the server).
For now, just never publish them.
Test Plan: Ran unit tests with notifications enabled but the server down and didn't get a bunch of warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10171
Summary:
Fixes T5476. Using edges to store which objects are on which board columns ends up being pretty awkward. In particular, it makes T4807 very difficult to implement.
Introduce a dedicated `BoardColumnPosition` storage.
This doesn't affect ordering rules (T4807) yet: boards are still arranged by priority. We just read which tasks are on which columns out of a new table.
Test Plan:
- Migrated data, then viewed some boards. Saw exactly the same data.
- Dragged tasks from column to column.
- Created a task directly into a column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5476
Differential Revision: https://secure.phabricator.com/D10160
Summary:
Restores functionality for Flowdock->Chatbot adapter.
Most likely the result of API changes in the year since the original patch was contributed,
the flowdock adapter no longer worked.
This makes a few tweaks to both the base streaming adapter class and the flowdock adpater. I took care to not disturb the functionality of the campfire adapter, but I don't have any way to test it.
Test Plan: I am new here and I have no idea what to write other than sarcastic things but I'll most like amend this after review.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10168
Summary:
Ref T4589. When you look at a file, we load attached objects in order to run the "you can see this if you can see any attached object" policy check.
However, right now the subquery inherits the "throw on filter" flag from the parent query. This inheritance makes sense in other cases[1], but because this is an "ANY" rule it does not make sense here. In practice, it means that if the file is attached to several objects, and any of them gets filtered, you can not see the file.
Instead, explicitly drop the flag for this subquery.
[1] Sort of. It doesn't produce wrong results in other cases, but now that I think about it might produce a less-tailored error than it could. I'll look into this the next time I'm poking around.
Test Plan:
- Viewed an "All Users" file attached to a private Mock.
- Prior to this patch, I incorrectly received an exception when the Mock was loaded. This is wrong; I should be able to see the file because the policy is "All Users".
- After the patch, I can correctly view the file, just not the associated mock.
{F127074}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: 20after4, aran, epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D8498
Summary: Ref T4659. Because we JOIN, tasks with no value are filtered out. Instead, LEFT JOIN.
Test Plan: Issued an "Order by" and got all applicable tasks. Adjusted values and saw order change.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10119
Summary:
Resolves T4659. This implements support for sorting tasks by custom fields.
Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query.
Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10106
Summary: Initially the change is aimed to solve issue with line breaks being lost in projects descriptions. But it is done in a general place so line breaks behavior is more consistent all over the place.
Test Plan:
- Write a multiline description of the project, using single \n for line breakers.
- View project details in project/view/X?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10014
Summary: Fixes T5102. Bumps the version to clear caches and documents the `#` headers.
Test Plan: Read documentation.
Reviewers: asherkin, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5102
Differential Revision: https://secure.phabricator.com/D10046
Summary:
Ref T4420. This was a performance hack introduced long ago to make typeaheads for users a little cheaper. The idea was that you could load some of an object's columns and skip other ones.
We now always load users on demand, so the cost of loading the whole objects is very small. No other use cases ever arose for this, and it seems unlikely that they will in the future. Remove it all.
Test Plan:
- Grepped for `CONFIG_PARTIAL_OBJECTS`.
- Grepped for `dirtyFields`.
- Grepped for `missingFields`.
- Grepped for `resetDirtyFields`.
- Grepped for `loadColumns`.
- Grepped for `loadColumnsWhere`.
- Grepped for `loadRawDataWhere`.
- Loaded and saved some lisk objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9895
Summary: Ref T5245. Earlier changes in this stack introduced new edge strings, provide English translations.
Test Plan: This is hard to test exhastively, but I poked around the UI and saw some of these rendering correctly. If I messed anything up it's trivial to fix when someone spots it.
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence, chad
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9920
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary: Ref T5245. With work elsewhere (notably, D9839) we can remove this TODO and use real transactions.
Test Plan: Pushed a `closes Txxx` commit and got a close + transaction.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9848
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary:
Ref T5245. See some discussion in D9838.
When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once.
To do this, allow edge types to `shouldWriteInverseTransactions()`. When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log.
Test Plan: Attached and detached revisions from tasks, saw transactions appear on both sides of the operation.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9839
Summary:
Ref T5245. A very long time ago I had this terrible idea that we'd let objects react to edges being added and insert transactions in response.
This turned out to be a clearly bad idea very quickly, for like 15 different reasons. A big issue is that it inverts the responsibilities of editors. It's also just clumsy and messy.
We now have `PhabricatorApplicationTransactionInterface` instead, which mostly provides a cleaner way to deal with this.
Implement `PhabricatorApplicationTransactionInterface`, implicitly moving all the attach actions (task/task, task/revision, task/commit, task/mock) to proper edge transactions.
The cost of this is that the inverse edges don't write transactions -- if you attach an object to another object, only the object you were acting on posts a transaction record. This is sort of buggy anyway already. I'll fix this in the next diff.
Test Plan: Attached tasks, revisions and mocks to a task, then detached them.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9838
Summary:
Ref T5245. I want to add a new capability to edge types, which is a good opportunity to move away from `PhabricatorEdgeConfig`, which isn't modular.
This is basically the same as the modularization of PHID types, which has worked well. Add `PhabricatorEdgeType` and provide an adaption layer for the existing code.
This has no runtime changes, except the fixed edge constant.
Test Plan: Ran `var_dump(PhabricatorEdgeType::getAllTypes())` and got reasonable looking output.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9837
Summary: Add a new line for clarity.
Test Plan: Inspected `./resources/celerity/map.php`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9931
Summary: `phutil_var_export` only accepts a single parameter. This bug was introduced in D9926.
Test Plan: Eye-ball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9930
Summary: Fixes T5336. Currently, `PhabricatorWorkerLeaseQuery` is basically FIFO. It makes more sense for the queue to be a priority-queue, and to assign higher priorities to alerts (email and SMS).
Test Plan: Created dummy tasks in the queue (with different priorities). Verified that the priority field was set correctly in the DB and that the priority was shown on the `/daemon/` page. Started a `PhabricatorTaskmasterDaemon` and verified that the higher priority tasks were executed before lower priority tasks.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5336
Differential Revision: https://secure.phabricator.com/D9871
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: The monospaced rule should still have higher precedence than these
rules, so use flat text tests to cover some rule interactions.
Auditors: btrahan
Summary: Remarkup rules can not safely use arbitrary text in tag attributes,
because it may include tokens which are later replaced. Precedence rules
should prevent this in general. Use flat text assertions and adjust precedence
rules in cases where they may not prevent tokens from appearing in attributes.
Auditors: btrahan
Summary:
Ref T3116. Add a Herald action "Require legal signatures" which requires revision authors to accept legal agreements before their revisions can be accepted.
- Herald will check which documents the author has signed, and trigger a "you have to sign X, Y, Z" for other documents.
- If the author has already signed everything, we don't spam the revision -- basically, this only triggers when signatures are missing.
- The UI will show which documents must be signed and warn that the revision can't be accepted until they're completed.
- Users aren't allowed to "Accept" the revision until documents are cleared.
Fixes T1157. The original install making the request (Hive) no longer uses Phabricator, and this satisfies our requirements.
Test Plan:
- Added a Herald rule.
- Created a revision, saw the rule trigger.
- Viewed as author and non-author, saw field UI (generic for non-author, specific for author), transaction UI, and accept-warning UI.
- Tried to accept revision.
- Signed document, saw UI update. Note that signatures don't currently //push// an update to the revision, but could eventually (like blocking tasks work).
- Accepted revision.
- Created another revision, saw rules not add the document (since it's already signed, this is the "no spam" case).
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: asherkin, epriestley
Maniphest Tasks: T1157, T3116
Differential Revision: https://secure.phabricator.com/D9771
Summary:
Ref T3116. Allow documents to be queried for ones the viewer has signed, and make this the default view.
This also relaxes the versioning stuff a little bit, and stops invalidating signatures on older versions of documents. While I think we should do that eventually, it should be more explicit and have better coordination in the UI. For now, we'll track and show older signatures, but not invalidate them.
I imagine eventually differentiating between "minor edits" (typo / link fixes, for example) and major edits which actually require re-signature.
Test Plan: {F171650}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D9769
Summary: Lightens up the border a little on obj tags
Test Plan: photoshop, homepage
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9759
Summary: Fixes T5497. Scope these down a little bit so they don't bleed into `{W...}` embeds and such.
Test Plan:
- Viewed a Legalpad document with headers, monospaced stuff, and lists. Looked the same before/after.
- Viewed a comment with headers, monospace, and lists. Looked the same before/after.
- Viewed a `{W..}` embed, now looks sane.
{F171052}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5497
Differential Revision: https://secure.phabricator.com/D9757
Summary: The rest of this code works if we hand off `array()`, and fataling here, while more correct, is harder for users to get out of (they have to go manually remove files) and not obvious.
Test Plan: Corrupted pid file and ran `phd stop`.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9749
Summary: Provides a base set of shaded object tags for use in Phabricator.
Test Plan:
Lots of Photoshop and Chrome.
{F170252, size=full}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9737
Summary: Moves PhabricatorActionHeaderView to PHUIActionHeaderView, adds Red, Green, and Violet colors and extend ObjectBox to take colors and action headers.
Test Plan:
Tested new Welcome layout as well as UIExamples, Workboards, and Hovercards
{F169669}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9707
Summary: Constructing tables manually just isn't fun.
Test Plan:
```
./bin/storage status
phabricator:db.audit Applied db audit
phabricator:db.calendar Applied db calendar
phabricator:db.chatlog Applied db chatlog
phabricator:db.conduit Applied db conduit
phabricator:db.countdown Applied db countdown
phabricator:db.daemon Applied db daemon
phabricator:db.differential Applied db differential
phabricator:db.draft Applied db draft
phabricator:db.drydock Applied db drydock
phabricator:db.feed Applied db feed
phabricator:db.file Applied db file
phabricator:db.flag Applied db flag
phabricator:db.harbormaster Applied db harbormaster
...
```
This probably isn't ready to land yet, we should fix `PhutilConsoleTable` to truncate columns which would otherwise cause overflow.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9604
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.
Test Plan: (See screenshots...)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T832, T4045, T5179
Differential Revision: https://secure.phabricator.com/D9294
Summary: Convert `./bin/mail` and a`./bin/sms` to use `PhutilConsoleTable` for formatting output.
Test Plan: I don't actually have mail and SMS setup on my dev box, but this is a pretty straightforward change.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9621
Summary:
Ref T4209. Unifies the local (`./bin/phd status`) and global (`./bin/phd status --all`) view into a single table. This generally makes it easy to administer daemons running across multiple hosts.
Depends on D9606.
Test Plan:
```
> sudo ./bin/phd status
ID Host PID Started Daemon Arguments
38 localhost 2282 Jun 18 2014, 7:52:56 AM PhabricatorRepositoryPullLocalDaemon
39 localhost 2289 Jun 18 2014, 7:52:57 AM PhabricatorGarbageCollectorDaemon
40 localhost 2294 Jun 18 2014, 7:52:57 AM PhabricatorTaskmasterDaemon
41 localhost 2314 Jun 18 2014, 7:52:58 AM PhabricatorTaskmasterDaemon
42 localhost 2319 Jun 18 2014, 7:52:59 AM PhabricatorTaskmasterDaemon
43 localhost 2328 Jun 18 2014, 7:53:00 AM PhabricatorTaskmasterDaemon
44 localhost 2354 Jun 18 2014, 7:53:08 AM PhabricatorRepositoryPullLocalDaemon X --not Y
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D9607
Summary: Ref T4209. Currently, `./bin/phd status` prints a table showing the daemons that are executing on the current host. It would be useful to be able to conventiently query the daemons running across all hosts. This would also (theoretically) make it possible to conditionally start daemons on a host depending upon the current state and on the daemons running on other hosts.
Test Plan:
```
> ./bin/phd status --all
ID Host PID Started Daemon Arguments
18 phabricator 6969 Jun 12 2014, 4:44:22 PM PhabricatorTaskmasterDaemon
17 phabricator 6961 Jun 12 2014, 4:44:19 PM PhabricatorTaskmasterDaemon
16 phabricator 6955 Jun 12 2014, 4:44:15 PM PhabricatorTaskmasterDaemon
15 phabricator 6950 Jun 12 2014, 4:44:14 PM PhabricatorTaskmasterDaemon
14 phabricator 6936 Jun 12 2014, 4:44:13 PM PhabricatorGarbageCollectorDaemon
13 phabricator 6931 Jun 12 2014, 4:44:12 PM PhabricatorRepositoryPullLocalDaemon
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D9497
Summary:
Full screen is a little foobar so disabling it for inline comments
Fixes T5272
Test Plan:
View inline comment after change, make sure full screen option
has gone.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5272
Differential Revision: https://secure.phabricator.com/D9579
Summary:
Ref T4986. Instead of requiring you to know engine class names and copy/paste URLs, provide select dropdowns that use SCARY JAVASCRIPT to do magical things.
I think this is mostly reasonable, the only issue is that it's hard to create a panel out of a completely ad-hoc query (you'd have to save it, then create a panel out of the saved query, then remove the saved query). Once we develop T5307 we can do a better job of this.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9572
Summary: Fixes T4729. This form is a little fluff, but we show it in the URI when you click an anchor on the page, and doing so seems desirable. I think it's reasonable to support this form, given that it appears in the URI.
Test Plan: Wrote some stuff like `M60`, `M60/71`, `M60/72/`, `M60/73/#13` and saw it all get picked up and rendered/linked properly.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4729
Differential Revision: https://secure.phabricator.com/D9555
Summary: Sometimes, the `PhabricatorInfrastructureTestCase` //would// fail, but we don't run it with `arc unit` because of the directory structure. See D9556 for an example.
Test Plan: This is essentially the same as D9557.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9559
Summary:
Ref T5359. When users upload non-image file types (PDFs, text files, whatever), Pholio currently chokes in a few places. Make most of these behaviors more reasonable:
- Provide thumbs in the required sizes.
- Predict the thumb size of these files correctly.
- Disable inline comments.
- Make "View Fullsize" and "Download" into buttons. These mostly-work. Download should probaly really download, but CSRF on forms is a bit of a pain right now.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5359
Differential Revision: https://secure.phabricator.com/D9548
Summary: This ensure that Aphlict is always served on the primary / production domain, because the alternate file domain is intended to prevent Flash execution.
Test Plan: Confirmed this fixes the issue by visiting https://code.redpointsoftware.com.au/ and seeing notification messages.
Reviewers: joshuaspence, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9511
Summary: This class has been deprecated for a while now (see rP0a8b0d1392bd79b4e88fbf910b176c960d57b4b4). It should be safe to remove.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9467
Summary: This class has been deprecated for a while now (see rPdad7c65bf56384480be7c18e02fdc01ea67cf1ff). It should be safe to remove.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9468
Summary: This class has been deprecated for a while (see rP574bc3ba31cca2767bafe7844d7f854d90d6be1c). It should be safe to remove now.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9469
Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.
Test Plan: Will run `arc unit` on another host.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9443
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary:
This currently uses a hard-coded relative path, but should not, especially after D9401.
The major effect of this is that updated .swf files might not be served properly, and we were at the whims of the server configuration for caching/versioning behavior.
Test Plan: Enabled debug notifications, saw .swf load through Celerity.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9421
Summary:
Replaces the icons with fonts from FontAwesome. Up in the air about the meme icon. Thoughts?
Also removed the second fullscreen/normal state. Seems obvious what it does, but assume someone complained previously?
Test Plan:
Tested all the icon states and made sure they still worked. Test fullscreen and help.
{F163485}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9385
Summary: Ref T2628. This makes Transactions understand objects that can have project relationships, extract project mentions, and handle watching.
Test Plan: See next diff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2628
Differential Revision: https://secure.phabricator.com/D9340
Summary: Builds a consistent 'selected, hover' state slightly darker than selected states.
Test Plan: Tested Conpherence, Sidenavs
Reviewers: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9345
Summary: Adds back the power icon
Test Plan: Logged out of local instance, saw icon appear. Click login icon. Logged in. Ate a toast sandwich.
Reviewers: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9336
Summary: Adds more consistent colors and spacing to notifications, conpherence dropdowns, search dropdowns, and typeaheads.
Test Plan: Tested Notifications, menu and page. Conpherence, menu and page, Search, and Typeaheads.
Reviewers: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9313
Summary: Removes lightblue app icons, moves the menu ones to menu sprite. Minor CSS updates to apps nav.
Test Plan: Test all sm icons work in new nav, test apps nav.
Reviewers: epriestley, btrahan
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9302
Summary: Fixes T5021, UI labels for the fields, "Edit Dependencies" in the action list, transaction strings ("added dependent tasks", etc), UI strings in the dependencies dialog (title/submit/etc)
Test Plan: Open task, edit blocks, dialog should have new term, task history should show "blocks" instead of "dependencies"
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5021
Differential Revision: https://secure.phabricator.com/D9270
Summary:
Ref T4657. Right now, you have to muck with `events.listeners` to install listeners. Instead, automatically install all subclasses of AutoEventListener.
Primarily, this makes it easier to resolve requests with "drop this file in `src/extensions/`, no warranty", which seems to have worked well so far in resolving things like custom remarkup rules, etc.
Test Plan:
- Added such a listener, had it autoregister.
- Clicked around and saw the effects of normal listeners.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4657
Differential Revision: https://secure.phabricator.com/D9262
Summary:
Fixes T4021. Chooses to keep a "primary" slug based off the name - including all that lovely logic - and allow the user to specify "additional" slugs. Expose these as "hashtags" to the user.
Sets us up for a fun diff where we can delete all the Project => Phriction automagicalness. In terms of this diff, see the TODOs i added.
Test Plan:
added a primary slug as an additional slug - got an error. added a slug in use on another project - got an error. added multiple good slugs and they worked. removed slugs and it worked. made some remark using multiple new slugs and they all linked to the correct project
ran epriestley's case
- Create project "A".
- Give it additional slug "B".
- Try to create project "B".
and i got a nice error about hashtag collision
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4021
Differential Revision: https://secure.phabricator.com/D9250
Summary:
Fixes T5094. In some cases we do slightly expensive transformations to resources (inlining images, replacing URIs, building packages). We can throw cache in front of them easily since URIs are already permanently associated with a single resource.
Also browse around and move some CSS/JS into packages.
Test Plan:
Added logging to verify the caches are working, saw moderately improved performance.
Browsed around looking at resources tab in developer console, saw fewer total requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5094
Differential Revision: https://secure.phabricator.com/D9175
Summary: placeholder text is pretty useful.
Test Plan: placeholder text is pretty useful. also fully supports not breaking everything.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9223
Summary: Fixes T4735. When running `./bin/phd`, show daemon arguments.
Test Plan:
```
./bin/phd status
PID Started Daemon Arguments
12711 May 20 2014, 9:02:52 AM PhabricatorRepositoryPullLocalDaemon []
12716 May 20 2014, 9:02:52 AM PhabricatorGarbageCollectorDaemon []
12733 May 20 2014, 9:02:53 AM PhabricatorTaskmasterDaemon []
12768 May 20 2014, 9:02:53 AM PhabricatorTaskmasterDaemon []
12775 May 20 2014, 9:02:53 AM PhabricatorTaskmasterDaemon []
12780 May 20 2014, 9:02:54 AM PhabricatorTaskmasterDaemon []
12838 May 20 2014, 9:02:54 AM PhabricatorFactDaemon []
13436 May 20 2014, 9:03:23 AM PhabricatorRepositoryPullLocalDaemon ["X","--not","Y"]
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4735
Differential Revision: https://secure.phabricator.com/D9208
Summary: Ref T4029. When checking the view policy of a document, require the viewer to also be able to see all of the ancestors.
Test Plan:
- Hard-coded `/x/y/` to "no one".
- Checked that `/x/y/` is not visible.
- Checked that `/x/y/z/` is not visible.
- Checked that `/x/`, `/x/q/`, etc., are still visible.
- Tested project pages and sub-pages for project visibility.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D9199
Summary:
Ref T4967. Adds a "Watch" relationship to projects, which is stronger than member/subscribed.
Specifically, when a task is tagged with a project, we'll include all project watchers in the email/notifications. Normally we don't include projects unless they're explicitly CC'd, or have some other active role in the object (like being a reviewer or auditor).
This allows you to closely follow a project without needing to write a Herald rule for every project you care about.
Test Plan:
- Watched/unwatched a project.
- Tested the watch/subscribe/member relationships:
- Watching implies subscribe.
- Joining implies subscribe.
- Leaving implies unsubscribe + unwatch.
- You can't unsubscribe until you unwatch (slightly better would be unsubscribe implies unwatch, but this is a bit tricky).
- Watched a project, then recevied email about a tagged task without otherwise being involved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4967
Differential Revision: https://secure.phabricator.com/D9185
Summary:
Updates policy, headers, typeaheads to FA over policy icons
Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late
Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names
Browsed numerous places, checked new dropdowns, see pudgy people.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4739
Differential Revision: https://secure.phabricator.com/D9179
Summary: In general these are fairly readable, but if not it cleans up on hover (and hover card).
Test Plan: tested a closed task in my sandbox
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9158
Summary:
D9153 fixed half of this, but exposed another issue, which is that we don't actually serve ".eot" and ".ttf" through Celerity right now.
Make sure we include them in the routes.
Test Plan:
- Downloaded CSS, JS, TTF, EOT, WOFF, JPG, etc., through Celerity.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9154
Summary:
See <6a45b7e670>
These URIs have "?hack=iefix#ieieielol" on them, which the parser doesn't recognize as a known resource, so it errs on the side of caution by not rewriting.
Instead, strip this bit off, attempt to rewrite, then put it back on.
Test Plan: Loaded `font-awesome.css` locally and saw properly rewritten URIs.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9153
Summary:
Fixes T5069. T2222 mostly-intentionally stopped emitting these.
My sense is that users generally find event listeners (or, really, writing PHP at all) much less preferable to things like Herald rules or HTTP hooks. This is generally good, since those things are way easier to maintain, so I plan to continue moving away from events in cases where we have reasonable alternatives.
We also generally have more and better alternatives now than when these were written.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5069
Differential Revision: https://secure.phabricator.com/D9151
Summary: Someone stole my bot's name, so the bot couldn't (re)connect. This tries adding some text to the name if it encounters a 433 'nick in use' error
Test Plan: Started a ton of bots: F154744
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9123
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.
Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9088
Summary: Remove white app icons, no longer in use as far as grep/memory serve. These were for list hover states.
Test Plan: Rebuild sprites, celerity. Grep for appIcon use (only feed). Verify all action lists are driven by FontAwesome.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9078
Summary: Ref T2683. Normally not a big deal, but if a readme has some codeblocks missing the cache can slow things down.
Test Plan:
- Verified we hit the cache.
- Verified TOC still works.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5028, T2683
Differential Revision: https://secure.phabricator.com/D9049
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary:
Also fix a few other minor issues:
- Use lint config.
- Fix a method signature from `arc unit --everything` (unrelated).
- Add a javelin doc.
Test Plan: Ran `arc lint`, `arc unit`, `arc linters`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9072
Summary:
Ref T2039. This diff is the equivalent to D9057, but for rP.
Depends on D9066.
Test Plan: Ran `arc lint` and ensure it doesn't complain about the `.arclint` file.
Reviewers: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9064
Summary:
Provides a working SMS implementation with support for Twilio.
This version doesn't really retry if we get any gruff at all. Future versions should retry.
Test Plan: used bin/sms to send messages and look at them.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: aurelijus, epriestley, Korvin
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D8930
Summary: Ref D8930. My "send test" for SMS was failing before this patch, and now it works nicely.
Test Plan: Used new code in D8930 that uses $this->queueTask() to get some work done and it got done in process
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9018
Summary: Ref T4119. Adds the block rule and makes a faint effort at CSS.
Test Plan: See D8953 for a screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8955
Summary:
Ref T4398. This prompts users for multi-factor auth on login.
Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one.
Test Plan:
- Used Conduit.
- Logged in as multi-factor user.
- Logged in as no-factor user.
- Tried to do non-login-things with a partial session.
- Reviewed account activity logs.
{F149295}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8922
Summary:
Ref T4843. This adds support to `javelin_tag()` for an `aural` attribute. When specified, `true` values mean "this content is aural-only", while `false` values mean "this content is not aural".
- I've attempted to find the best modern approaches for marking this content, but the `aural` attribute should let us change the mechanism later.
- Make the "beta" markers on application navigation visual only (see T4843). This information is of very low importance, the application navigation is accessed frequently, and the information is available on the application list.
- Partially convert the main navigation. This is mostly to test things, since I want to get more concrete feedback about approaches here.
- Add a `?__aural__=1` attribute, which renders the page with aural-only elements visible and visual-only elements colored.
Test Plan: {F146476}
Reviewers: btrahan, scp, chad
Reviewed By: chad
Subscribers: aklapper, qgil, epriestley
Maniphest Tasks: T4843
Differential Revision: https://secure.phabricator.com/D8830
Summary:
Ref T3583. Adds edges, query relationships, etc. Lots of debugging/temporary UI.
My general intent here is to use edges to track where panels appear, and then put additional data on the dashboard itself to control layout, positioning, etc.
Dashboards don't actually render yet so this is still pretty boring.
Test Plan:
{F149175}
{F149176}
{F149177}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8916
Summary:
Moderize Inline Comment Display
- Use standard colors
- Better display with/without comment
- OMG Icons
Test Plan:
{F148256}
Test with and without main comment, test with many for few comments on 1-3 files.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8885
Summary:
In some applications, using `{V2}` syntax to embed a vote throws. The chain of causality looks like this:
- We try to render a `phabricator_form()`.
- This requires a CSRF token.
- We look for a CSRF token on the user.
- It's an omnipotent user with no token, so everything fails.
To resolve this, make sure we always pass the real user in.
Test Plan:
- Lots of `grep`.
- Made a Differential comment with `{V2}`.
- Made a Diffusion comment with `{V2}`.
- Made a Maniphest comment with `{V2}`.
- Replied to a Conpherence thread with `{V2}`.
- Created a Conpherence thread with `{V2}`.
- Used Conduit to update a Conpherence thread with `{V2}`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lkassianik
Differential Revision: https://secure.phabricator.com/D8849
Summary: Ref T3718. Move from unbatched / ad-hoc loading to standard stuff for handles.
Test Plan: Looked at some requests and saw no changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D8810
Summary: This `%d` should be a `%s`, since the `PhutilNumber` value may get formatted according to locale settings.
Test Plan: will make @zeeg
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D8814
Summary:
PHP 5.5 specifies constant PASSWORD_BCRYPT should be used in password_hash()
instead of CRYPT_BLOWFISH. Using CRYPT_BLOWFISH is not supported in either PHP
or HHVM. This constant breaks Username / Password authentication.
Test Plan:
Login using Username/Password with bcrypt hash. Before applying the patch,
No matter what password entered, it will always fail authentication. After this
patch, user should be able to login with bcrypt hash.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8808
Summary: This adds in the Glyphicons Halflings Font/Iconset as an option for PHUIIconView along with a standard set of 10 colors. This will be a replacement for the standard action icon set in upcoming diffs, as well as obviously give us more flexibility, less KB, and less design resource time managing images.
Test Plan: UIExamples, Diviner
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8798
Summary:
For Harbormaster tasks which want to poll or wait, this lets them say "try again a little later" without having to sleep and hold a queue slot.
This is basically the same as failing, except that we don't increment the failure counter. Instead, we just set the current lease to the correct length and then exit. The task will be retried after the lease expires.
Test Plan: Using both `bin/harbormaster` and `phd debug taskmaster`, ran a lot of waiting tasks through the queue, faking them to either yield or not yield in a controlled manner. The queue responded as expected, yielding tasks appropraitely and retrying them later.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8792
Summary:
Fixes T4802. For context, see T1921.
Originally (in T1921), a developer ran into an issue where rendering `phabricator_form()` with an absolute URI confusingly dropped CSRF tokens, and it wasn't obvious why. This is a security measure, but at the time it wasn't very clear how all the pieces fit together. To make it more clear, we:
# expanded the exception text in developer mode to include a description of this issue; and
# added an exception in developer mode when rendering a form like this.
However, (2) causes some undesirable interactions for file downloads. In particular, if:
- developer mode is on; and
- there's no alternate file domain configured; and
- you try to download a file...
...we produce CDN URIs that are fully-qualified, and you get the exception from (2) above.
This is kind of a mess, and producing fully-qualified CDN URIs in all cases is simple and clear and desirable. To resolve this, just revert (2). We still have the clarification from (1) above and this hasn't caused further issues, so I think that's sufficient. This is a rare issue anyway and not particularly serious or error prone (at worst, a bit confusing and annoying, but hopefully easy to understand and resolve after the changes in (1)).
Test Plan: With develper mode and no alternate file domain, downloaded files from Files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4802
Differential Revision: https://secure.phabricator.com/D8776
Summary:
See discussion in D8773. Three small adjustments which should help prevent this kind of issue:
- When queueing followup tasks, hold them on the worker until we finish the task, then queue them only if the work was successful.
- Increase the default lease time from 60 seconds to 2 hours. Although most tasks finish in far fewer than 60 seconds, the daemons are generally stable nowadays and these short leases don't serve much of a purpose. I think they also date from an era where lease expiry and failure were less clearly distinguished.
- Increase the default wait-after-failure from 60 seconds to 5 minutes. This largely dates from the MetaMTA era, where Facebook ran services with high failure rates and it was appropriate to repeatedly hammer them until things went through. In modern infrastructure, such failures are rare.
Test Plan:
- Verified that tasks queued properly after the main task was updated.
- Verified that leases default to 7200 seconds.
- Intentionally failed a task and verified default 300 second wait before retry.
- Removed all default leases shorter than 7200 seconds (there was only one).
- Checked all the wait before retry implementations for anything much shorter than 5 minutes (they all seem reasonable).
Reviewers: btrahan, sowedance
Reviewed By: sowedance
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8774
Summary:
Fixes T4773. For config settings of type `list<string>`, `set`, or `list<regex>`, the "defaults" table and "examples" aren't always in the same format you should actually use when changing the setting.
This is pretty confusing. Instead, always show the settings in the desired format. For example, if the user should enter a newline-separated list, show them a newline separated list.
Test Plan:
- Grepped for `list<string>`, `list<regex>`, and `'set'`; verified all the config had the right example format (most already did).
- Viewed config settings of various kinds, including custom settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4773
Differential Revision: https://secure.phabricator.com/D8725
Summary:
This adds a system which basically keeps a record of recent actions, who took them, and how many "points" they were worth, like:
epriestley email.add 1 1233989813
epriestley email.add 1 1234298239
epriestley email.add 1 1238293981
We can use this to rate-limit actions by examining how many actions the user has taken in the past hour (i.e., their total score) and comparing that to an allowed limit.
One major thing I want to use this for is to limit the amount of error email we'll send to an email address. A big concern I have with sending more error email is that we'll end up in loops. We have some protections against this in headers already, but hard-limiting the system so it won't send more than a few errors to a particular address per hour should provide a reasonable secondary layer of protection.
This use case (where the "actor" needs to be an email address) is why the table uses strings + hashes instead of PHIDs. For external users, it might be appropriate to rate limit by cookies or IPs, too.
To prove it works, I rate limited adding email addresses. This is a very, very low-risk security thing where a user with an account can enumerate addresses (by checking if they get an error) and sort of spam/annoy people (by adding their address over and over again). Limiting them to 6 actions / hour should satisfy all real users while preventing these behaviors.
Test Plan:
This dialog is uggos but I'll fix that in a sec:
{F137406}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8683
Summary: IE won't load background images in a page that are served with the mimetype "image/jpg" as it only recognises the "image/jpeg" mimetype.
Test Plan: Spent an hour or two going back and forth between Linux (to dev) and Windows (to test) to find the source of this issue, then flipped several tables at IE for being terrible.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8689
Summary: Ref T418. Fixes T4642. The "changes since last update" and "branch" fields got dropped; restore them in a general, field-driven way.
Test Plan:
- Created a revision, got relevant sections in mail.
- Commented on a revision, got relevant sections in mail.
- Updated a revision, got relevant sections in mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: spicyj, epriestley
Maniphest Tasks: T418, T4642
Differential Revision: https://secure.phabricator.com/D8657
Summary: Fixes T4683. This was just a missing method implementation. Also provide a couple of translation things.
Test Plan:
- Created a revision from the command line with a nonempty `JIRA Issues:` line, via `arc diff`.
- Looked at the translation strings.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4683
Differential Revision: https://secure.phabricator.com/D8656
Summary:
Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries.
For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed.
Overall, this is basically the same as commit email, but more suitable for some workflows.
Test Plan:
Wrote some rules, then made a bunch of pushes. Got email like this:
{F134929}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8618
Summary: Ref T4590. Ref T1049. This is primarily intended to support HTTP auth in Harbormaster.
Test Plan: Added a field, edited it, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4590, T1049
Differential Revision: https://secure.phabricator.com/D8607
Summary:
Ref T1049. Fixes T4602. Moves all the funky field stuff to CustomField. Uses ApplicationTransactions to apply and record edits.
This makes "artifact" fields a little less nice (but still perfectly usable). With D8599, I think they're reasonable overall. We can improve this in the future.
All other field types are better (e.g., fixes weird bugs with "bool", fixes lots of weird behavior around required fields), and this gives us access to many new field types.
Test Plan:
Made a bunch of step edits. Here's an example:
{F133694}
Note that:
- "Required" fields work correctly.
- the transaction record is shown at the bottom of the page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4602, T1049
Differential Revision: https://secure.phabricator.com/D8600
Summary: Ref T4663. Ref T4659. Allows "date" fields to be filtered with range parameters.
Test Plan:
- Added a custom "date" field with "search".
- Populated some values.
- Searched for dates using new range filters.
- Combined date search with other searches.
- Ran other searches independently.
- Inspected the generated queries.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: shadowhand, epriestley
Maniphest Tasks: T4659, T4663
Differential Revision: https://secure.phabricator.com/D8598
Summary:
- Point them at the new Diviner.
- Make them a little less cumbersome to write.
Test Plan: Found almost all of these links in the UI and clicked them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8553
Summary:
Fixes T3976. Long ago, some applications used "#comment-5" instead of "#5" for transaction/comment anchors. Now everything (I think?) uses "#5"; this is the style used by ApplicationTransactions.
This might break some very old, explcit `T123#comment-5` links, or off-site links to the `comment-N` anchors, but all that stuff generally got renumbered when we migrated anyway and getting you to the right object is like 95% of the job.
Test Plan: Verified that `T123#5` now links to `#5`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3976
Differential Revision: https://secure.phabricator.com/D8542
Summary: Ref T4430. This just deploys it on the property lists. (Help on how to do translations better? I tried a more traditional pht('%s, %s, %s, and %d other(s)') but I think the string lookup assumes the %d comes as the second param or something?)
Test Plan: Made a Maniphest Task with a hojillion subscribers and noted the working dialogue. Also made a Pholio Mock with lots of subscribers and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin, chad
Maniphest Tasks: T4430
Differential Revision: https://secure.phabricator.com/D8525
Summary: Fixes T4614. These don't do anything bad or dangerous, but generate unusable pages.
Test Plan:
- Added and executed unit tests.
- Tried to create pages like `/../`, `/begin/../end/`, etc.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: aran, epriestley
Maniphest Tasks: T4614
Differential Revision: https://secure.phabricator.com/D8535
Summary: Ref T4587. Add an option to automatically generate a keypair, associate the public key, and save the private key.
Test Plan: Generated some keypairs. Hit error conditions, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4587
Differential Revision: https://secure.phabricator.com/D8513
Summary:
Ref T4588. Request from @zeeg. Adds a "BRANCHES" field to commit emails, so the branches the commit appears on are shown.
I've implemented this with CustomField, but in a very light way.
Test Plan: Used `scripts/repository/reparse.php --herald` to generate mail, got a BRANCHES section where applicable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley, zeeg
Maniphest Tasks: T4588
Differential Revision: https://secure.phabricator.com/D8501
Summary: see title. Fixes T4549.
Test Plan: made a readme that had some headers and observed a nice ToC
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Maniphest Tasks: T4549
Differential Revision: https://secure.phabricator.com/D8490
Summary:
Ref T2222.
- Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
- Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
- Moves Releeph churn field off `DifferentialCommentQuery`.
- Removes dead code in `DifferentialRevisionViewController`.
- Removes `DifferentialException` (no references).
- Removes `DifferentialRevision->loadComments()` (no callsites).
- Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
- Removes `DifferentialCommentQuery` (all callsites updated).
Test Plan: Mostly a lot of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8476
Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.
This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.
Test Plan: Used `arc diff --create` to create several revisions with different data.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8452
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary: Ref T4570. Add trivial assertions to tests which fail-by-exploding so we can fail tests with no assertions.
Test Plan: Ran `arc unit --everything` with Arcanist patched to fail with no assertions.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8436
Summary:
- Allow Celerity to map and serve WOFF files.
- Add Source Sans Pro, Source Sans Pro Bold, and the corresponding LICENSE.
- Add a `font-source-sans-pro` resource for the font.
Test Plan:
- Changed body `font-face` to `'Source Sans Pro'`.
- Added `require_celerity_resource('font-source-sans-pro')` in StandardPageView.
Works in Firefox/Chrome/Safari, at least:
{F123296}
{F123297}
{F123298}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D8430
Summary: Adds support for custom SSL certs in the IRC bot config, same as in .arcconfig
Test Plan: Bot wouldn't connect before. Added this code and corresponding line in bot config, now it does.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8393
Summary:
A few minor fixes:
- When we build a tag with `"meta" => null`, strip the attribute like we do for all other attributes. Previously, we would actually set the metadata to `null`. This happened with the Conpherence form.
- Just respond to the draft request with an empty (but valid) response, instead of building a dialog.
- `PhabricatorShapedRequest` is confusingly named and I should have caught this in review, but the basic shape of it is:
- You make one object.
- You call `trigger()` when stuff changes (e.g., a keystroke).
- It manages making a small number of requests (e.g., one request after the user stops typing for a moment).
- The way it was being used previously would incorrectly send a request for every keystroke.
I think I'm going to simplify `ShapedRequest` and merge it into some larger queue for T430.
Test Plan: Typed some text, no longer saw a flurry of requests. Reloaded page, still saw draft text.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D8380
Summary:
Ref T2222. Differential has certain "words of power" (like `Ref T123` or `Depends on D345`) which should expand into a separate transaction when they appear anywhere in text.
Currently, they're respected in only some fields. I'm expanding them to work in any remarkup field, including comments and inline comments.
This partially generalizes transaction expansion/extraction in comments. Eventually, I'll probably implement some very soft sort of reference edge for T4036, maybe.
Test Plan: {F119368}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8369
Summary: Better aligns the text area when leaving an inline comment. Also, phts
Test Plan: reload page, view new padding.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8370
Summary:
Ref T2222. Ref T3886. Gets the storage-based fields working.
This requires future changes to actually do anything, all this code is inactive.
Test Plan: {F118863}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8357
Summary:
Ref T2222. This isn't complete and doesn't change runtime behavior yet (the new fields are not glued to the interface), but implements many fields.
(The remaining fields have something weird going on with them, for the most part.)
Test Plan:
With additional changes, rendered most fields sensibly:
{F118834}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8354
Summary:
Ref T2222. Ref T3886. Ref T418. A few changes:
- CustomField can now index into global search.
- Use CustomField fields instead of older custom fields for Differential global search. (This slightly breaks any custom fields which exist, but they are presumably very rare, and probably do not exist; this break is also very mild.)
- Automatically perform CustomField and Subscribable indexing on applicable object types.
Test Plan: Used `bin/search index` to reindex a bunch of stuff, then searched for it. Debug-dumped abstract documents to inspect them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886, T2222
Differential Revision: https://secure.phabricator.com/D8346
Summary:
Ref T2222. This introduces two small new concepts:
- `expandTransactions()`: allows a transaction to expand into several transactions. For example, "resign" adds a "remove reviewers" transaction.
- We have some other cases which could use this, but currently hard-code things outside of the `Editor`.
- One example is that in Maniphest, closing a task implies claiming it if it is unowned.
- `setIgnoreOnNoEffect()`: The whole Editor can be set to continue or stop if any transactions have no effect, but this allows the behavior to be refined at the individual transaction level. This is primarily to make the UX less confusing, so the user gets only a single relevant error instead of one for each expanded transaction.
Otherwise, this is pretty straightforward.
Test Plan:
Rigged comment form to use SavePro controller, enabled resign action, then tried to resign from a bunch of stuff.
{F117743}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8328
Summary:
Ref T1191. I believe we only have three meaningful binary fields across all applications:
- The general cache may contain gzipped content.
- The file storage blob may contain arbitrary binary content.
- The Passphrase secret can store arbitrary binary data (although it currently never does).
This adds Lisk config for binary fields, and uses `%B` where necessary.
Test Plan:
- Added and executed unit tests.
- Forced file uploads to use MySQL, uploaded binaries.
- Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
- Tried to register with an account containing a G-Clef, and was stopped before the insert.
Reviewers: btrahan, arice
Reviewed By: arice
CC: arice, chad, aran
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D8316
Summary:
Ref T1191. Test that MySQL's rules match those of `phutil_is_utf8_with_only_bmp_characters()`:
- Build a string with //every// character that we consider to be a BMP character.
- Write it into MySQL.
- Read it back out.
- Make sure MySQL didn't truncate it.
Test Plan: Ran unit test. This test runs pretty quickly (50ms), the string with every character isn't all that enormous.
Reviewers: btrahan, arice
Reviewed By: arice
CC: chad, arice, aran
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D8314
Summary:
Ref T3886. Broadly, fields break down into two types right now: fields which store data on the object (like `DifferentialTitleField`) and fields which store data in custom field storage.
The former type generally reads data from the object into local storage prior to editing, then writes it back afterward. Currently, this happens in `didSetObject()`.
However, now that we load and set objects from ApplicationTransactionQuery, we'll do this extra read-field-values on view interfaces too. There, it's unnecessary and sometimes throws data-attached exceptions.
Instead, separate these concepts, and do all the read-from-object / read-from-storage in one logical chunk, separate from `didSetObject()`.
Test Plan:
- Edited Differential revision.
- Edited Maniphest task.
- Edited Project.
- Edited user profile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8299
Summary:
Ref T3886. Ref T418.
- Adds "View Policy" and "Edit Policy" fields.
- Allows CustomFields to produce arbitrary types of transactions, so these fields can produce standard view/edit policy transactions and get all the strings and validation associated with them.
Test Plan: {F116001}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886
Differential Revision: https://secure.phabricator.com/D8287
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.
Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.
Test Plan:
{F115918}
- Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T418
Differential Revision: https://secure.phabricator.com/D8284
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8294
Summary: Fixes T4466. We do an excessively strict effect check now, which means that these fields changing from (string) "16" to (int) 16 generates a transaction. Instead, compare integer values if the field has data in it.
Test Plan:
{F116261}
(Also made updates without changing the number, which did not appear in the transaction log anymore.)
Reviewers: btrahan, richardvanvelzen
Reviewed By: richardvanvelzen
CC: aran
Maniphest Tasks: T4466
Differential Revision: https://secure.phabricator.com/D8292
Summary:
Fixes T4463. When your VCS or account password is not set, we test it for upgrade anyway. This doesn't make sense and throws shortly into the process because the empty hash isn't parseable.
Instead, only show upgrade prompts when the password exists.
Test Plan:
- Added a password to an existing account with no password via password reset.
- Added a VCS password to an existing account with no VCS password.
- Observed no fatals / nonsense behaviors.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4463
Differential Revision: https://secure.phabricator.com/D8282
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.
Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8275
Summary: Fixes T4443. Plug VCS passwords into the shared key stretching. They don't use any real stretching now (I anticipated doing something like T4443 eventually) so we can just migrate them into stretching all at once.
Test Plan:
- Viewed VCS settings.
- Used VCS password after migration.
- Set VCS password.
- Upgraded VCS password by using it.
- Used VCS password some more.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8272
Summary:
Ref T4443. In addition to performing upgrades from, e.g., md5 -> bcrypt, also allow sidegrades from, e.g., bcrypt(cost=11) to bcrypt(cost=12). This allows us to, for example, bump the cost function every 18 months and stay on par with Moore's law, on average.
I'm also allowing "upgrades" which technically reduce cost, but this seems like the right thing to do (i.e., generally migrate password storage so it's all uniform, on average).
Test Plan:
- Fiddled the bcrypt cost function and saw appropriate upgrade UI, and upgraded passwords upon password change.
- Passwords still worked.
- Around cost=13 or 14 things start getting noticibly slow, so bcrypt does actually work. Such wow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8271
Summary:
Ref T4443.
- Add a `password_hash()`-based bcrypt hasher if `password_hash()` is available.
- When a user logs in using a password, upgrade their password to the strongest available hash format.
- On the password settings page:
- Warn the user if their password uses any algorithm other than the strongest one.
- Show the algorithm the password uses.
- Show the best available algorithm.
Test Plan: As an md5 user, viewed password settings page and saw a warning. Logged out. Logged in, got upgraded, no more warning. Changed password, verified database rehash. Logged out, logged in.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8270
Summary:
Ref T4443. Make hashing algorithms pluggable and extensible so we can deal with the attendant complexities more easily.
This moves "Iterated MD5" to a modular implementation, and adds a tiny bit of hack-glue so we don't need to migrate the DB in this patch. I'll migrate in the next patch, then add bcrypt.
Test Plan:
- Verified that the same stuff gets stored in the DB (i.e., no functional changes):
- Logged into an old password account.
- Changed password.
- Registered a new account.
- Changed password.
- Switched back to master.
- Logged in / out, changed password.
- Switched back, logged in.
- Ran unit tests (they aren't super extensive, but cover some of the basics).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, kofalt
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8268
Summary: Fixes T3872. Ref T1812. Ref T3886. Modernize the "closes x as y" string parser, and use all the new parsers instead of the old ones.
Test Plan: Made a commit full of a pile of these trigger strings, then used `scripts/repository/reparse.php --message` to reparse it. Verified that parses came back as expected using a bunch of `var_dump()`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1812, T3872, T3886
Differential Revision: https://secure.phabricator.com/D8263
Summary:
Ref T3886. See D8261. This brings the "reverts x" phrase to modern infrastructure. It isn't actually called by the real parser yet, I'm going to do that in one go at the end so I can test everything more easily.
This had unit tests; port most of them forward.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8262
Summary:
Ref T3886. Ref T3872. Ref T1812. We have several parsers which look for textual references to other objects, like:
Closes Tx.
Depends on Dy.
Reverts Dz.
Currently, these are pretty hard coded, don't get all the edge cases right, and don't generalize well. They're also implemented in the middle of Differential's field code. So I want to:
- Share more code so that, e.g., "Tx, Ty" always works (only some rules support it right now);
- fix bugs in the parser, like T3872;
- make this a modular, extensible process which runs against custom fields, not a builtin part of fields;
- make the internals more flexible to accommodate custom stuff like T1812.
This implements the "Verbs optional-noun Object, Optional Other Objects optional-as-something." grammar in a general way so subclasses can just plug in their keywords. Runtime code doesn't touch this yet.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3872, T1812, T3886
Differential Revision: https://secure.phabricator.com/D8261
Summary:
Allows to keep really wide graphs inside the div:
dot (width=100%) {{{ }}}
Test Plan:
Created a graph that is wider than a phriction page.
Added `(width=100%)` and now the images stays within the div.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8235
Summary:
Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions.
This might need some #design help.
Test Plan:
The tricky part here is the anchor rule, which deals with the case where you follow a link to `T123#4`, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest:
{F112891}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8229
Summary: Fixes T3300. Images under 32KB are inlined automatically into CSS using data URIs; larger images remain as normal links. I picked the 32KB threshold arbitrarily, based on it looking roughly like it got reasonable results on `core.pkg.css` (inlining most of the random stuff, but not inlining all the 2X sprites and such).
Test Plan: Loaded site, browsed around, looked at generated CSS.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3300
Differential Revision: https://secure.phabricator.com/D8225
Summary: Ref T4375. We never join this table, so this is a pretty straight find/replace.
Test Plan: Browsed around Calendar, verified that nothing seemed broken. Saw my red dot in other apps.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8145
Summary: Ref T1139. This has some issues and glitches, but is a reasonable initial attempt that gets some of the big pieces in. We have about 5,200 strings in Phabricator.
Test Plan: {F108261}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D8138
Summary: `What's new` has been broken for awhile, I've updated it to use the `feed.query` text view.
Test Plan: Start up a bot and say "What's new?"
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: fas, epriestley, aran, Kage, demo
Differential Revision: https://secure.phabricator.com/D8118
Summary: This adds the app icons, cleans up css Ref T3623
Test Plan: see new icons in dropdown menu
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8124
Summary:
Ref T3583. General idea here is:
- Users will be able to create `DashboardPanel`s, which are things like the jump nav, or a minifeed, or recent assigned tasks, or recent tokens given, or whatever else.
- The `DashboardPanel`s can be combined into `Dashboard`s, which select specific panels and arrange them in some layout (and maybe have a few other options eventually).
- Then, you'll be able to set a specific `Dashboard` for your home page, and maybe for project home pages. But you can also use `Dashboard`s on their own if you just like dashboards.
My plan is pretty much:
- Put in basic infrastructure for dashboards (this diff).
- Add basic create/edit (next few diffs).
- Once dashboards sort of work, do the homepage integration.
This diff does very little: you can't create dashboards or panels yet, and thus there are no dashboards to look at. This is all skeleton code, pretty much.
IMPORTANT: We need an icon bwahahahahaha
Test Plan:
omg si purrfect
{F106367}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8109
Summary: Ref T1921. Ref T4339. If you `phabricator_form()` with an absolute URI, we silently drop the CSRF tokens. This can be confusing if you meant to specify `"/some/path"` but ended up specifying `"http://this.install.com/some/path"`. In all current cases that I can think of / am aware of, this indicates an error in the code. Make it more obvious what's happening and how to fix it. The error only fires in developer mode.
Test Plan: Hit this case, also rendered normal forms.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4339, T1921
Differential Revision: https://secure.phabricator.com/D8044
Summary: we need this for legalese. Ref T3116
Test Plan: made a legalpad document with underlines. also re-gened docs and noted underlines worked
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7996
Summary:
Ref T4327. I want to make change parsing testable; one thing which is blocking this is that the Git discovery process is still part of `PullLocal` daemon instead of being part of `DiscoveryEngine`. The unit test stuff which I want to use for change parsing relies on `DiscoveryEngine` to discover repositories during unit tests.
The major reason git discovery isn't part of `DiscoveryEngine` is that it relies on the messy "autoclose" logic, which we never implemented for Mercurial. Generally, I don't like how autoclose was implemented: it's complicated and gross and too hard to figure out and extend.
Instead, I want to do something more similar to what we do for pushes, which is cleaner overall. Basically this means remembering the old branch heads from the last time we parsed a repository, and figuring out what's new by comparing the old and new branch heads. This should give us several advantages:
- It should be simpler to understand than the autoclose stuff, which is pretty mind-numbing, at least for me.
- It will let us satisfy branch and tag queries cheaply (from the database) instead of having to go to the repository. We could also satisfy some ref-resolve queries from the database.
- It should be easier to extend to Mercurial.
This implements the basics -- pretty much a table to store the cursors, which we update only for Git for now.
Test Plan:
- Ran migration.
- Ran `bin/repository discover X --trace --verbose` on various repositories with branches and tags, before and after modifying pushes.
- Pushed commits to a git repo.
- Looked at database tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7982
Summary:
does a few smallish things... Ref T3116
- adds an action to "sign document", thus improving visiblity of this feature from 0 to some value more than 0
- adds a crumb on the edit page to get back to the view page
- warns the user on the edit page IFF signatures exist for the current version that their edits could invalidate those signatures
- adds a "needSignatures" option to the Document Query class
Test Plan: click the new UI elements and they worked. edited a document with signatures, noted warning UI, edited anyway, noted warning UI correctly disappeared on new edit. also verified a single signature had the correct translation
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7983
Summary: This modularizes the rest of the GC submethods. Turned out there was nothing tricky.
Test Plan: Ran `bin/phd debug garbage` and got reasonable looking behavior and output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7971
Summary:
The GC is a big block of hard-coded application GCs right now. Among other things, this means third parties can't tap into the infrastructure.
Modularize it into `GarbageCollector` classes. This implements only one to prove the new stuff works; I'll followup with the rest in the next diff or few depending on how much mess I run into.
Test Plan: Used `bin/phd debug garbage` to run the collector in debug mode, observed reasonable output and behavior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7970
Summary: Fixes T4317. Update the "inline comment" control to a RemarkupControl. This could maybe use some padding/spacing/design touches eventually but seems OK for the moment.
Test Plan: {F101825}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4317
Differential Revision: https://secure.phabricator.com/D7969
Summary: Ref T1344. Write edges and read them when reloading the board.
Test Plan: After reload, stuff stays mostly where I put it. In-column order isn't always persisted correctly yet.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7944
Summary:
Ref T1344. This fixes two issues with DraggableList:
- In lists which allowed it, you could drag the top item above itself and get a dashed-border ghost item. This didn't make sense and didn't behave well. Just don't treat this operation as valid.
- In lists which allowed it, you could drag any non-top item to the topmost position, then drag it to an invalid position. The dashed-border ghost item would not be removed properly if this happend.
- Also fix some minor leftovers with Celerity.
Test Plan:
- Dragged the first item above itself; now an invalid operation with no ghost.
- Dragged another item to the first position then back to its original position; ghost vanishes.
- Clean lint.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7939
Summary:
Currently, to add new migration patches you need to:
- Add a file to `resources/sql/patches/`; then
- add an entry to `src/infrastructure/storage/blahblah/BlahBlahBlah.php`.
The second step isn't actually necessary, and we've been using this system for a long time without any issues arising.
Instead of requiring manual adjustments to the patch list, infer the patch specifications from the files on disk so you don't need to do step 2.
Also, simplify the existing data, which can //mostly// be derived from patch names. There are a few exceptions/errors, noted inline, which are preserved for compatibility.
Test Plan:
- For the new genration of `name` and `type`, I added code to check that the old and new values were the same before converting. This caught the two inline exceptions ("emailtableport", "drydockresouces").
- Added new patches to `autopatches/` and ran `bin/storage status` to verify they got picked up correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7894
Summary: Ref T4264. Ref T2628. Ref T3102. Allows you to associate repositories with projects. In the future, you'll be able to write Herald object rules against projects, use Herald fields like "Repository's projects", and search by project.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3102, T4264, T2628
Differential Revision: https://secure.phabricator.com/D7881
Summary: Ref T4222. Adds the map name to Celerity resource URIs, so we can serve out of any map.
Test Plan: Poked around, verified URIs have "/phabricator/" in them now.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7877
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.
Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.
Test Plan:
- Reloaded pages.
- Browsed around Differential.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7876
Summary:
Repositories currently have a no-UI "shortcut" feature which is only used by Facebook (and I'm not sure it's even used). As implemented, this feature is policy-oblivious and kind of nonsensical. Throw it away.
I'm open to reimplementing this, but I want to see some level of interest in it before I do. The new implementation would add shortcuts to each repository, similar to how mirrors work. My original plan was to follow this up with such an implementation (it's half-implemented in my sandbox), but as I worked through it I'm not sure it's really valuable.
Test Plan: Browsed repository list, grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D7862
Summary:
Ref T4222. Earlier, I adjusted the root from `webroot/` to `webroot/rsrc/`. However, this means that all the `/rsrc/x/y/z.jpg` fragments in CSS are no longer recognized as resource names.
Since we have like 9,000 things in CSS that do `url(/rsrc/xyz.jpg)` and I don't want to fix/test them all, so just make them work as-is. There's no real reason either setting is better than the other.
(Both URLs also work fine, but the parsed one will be better once we have real CDN support.)
Test Plan: Verified CSS gets managed resource URIs transformed into it.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7875
Summary: Ref T4222. This doesn't actually support multiple sources yet, but moves us closer by getting rid of some dead and exceedingly-singletoney code.
Test Plan: Browsed around, looked at Phame blogs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7874
Summary:
Ref T4222. This fixes some issues with Phame's resource construction.
Phame requires a fully virtual resource source, and since I want to run wordpress templates unmodified some day I don't want to build resource maps for skins.
Move all the stuff that depends on resource lists being discoverable at build time to `CelerityPhysicalResources`, and only generate maps for subclasses.
The root `CelerityResources` can now construct virtual resources; construct a virtual resource for Phame and use it.
Test Plan: Off-domain blogs work correctly now. On-domain blogs with custom skins work correctly now.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7873
Summary:
Ref T4222.
- Removes the old map and changes the CelerityResourceMap API to be entirely driven by the new map.
- The new map is about 50% smaller and organized more sensibly.
- This removes the `/pkg/` URI component. All resources are now required to have unique names, so we can tell if a resource is a package or not by looking at the name.
- Removes some junky old APIs.
- Cleans up some other APIs.
- Added some feedback for `bin/celerity map`.
- `CelerityResourceMap` is still a singleton which is inextricably bound to the Phabricator map; this will change in the future.
Test Plan:
- Reloaded pages.
- Verified packaging works by looking at generated includes.
- Forced minification on and verified it worked.
- Forced no-timestamps on and verified it worked.
- Rebuilt map.
- Ran old script and verified error message.
- Checked logs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: chad, aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7872
Summary: Ref T4222. These are the last two "return a big ball of mud" methods. Make the API stronger so I can swap out the implementations.
Test Plan: Reloaded pages.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7871
Summary:
Ref T4222. A few diffs from now, `CelerityResourceMap` will have a `CelerityResources` inside of it:
- Rename `resolvePackage()` to `getResourceNamesForPackageHash()`. This isn't a functional change, it's just making it clear what it does.
- Add `getResourceDataForName()`, to push details about storage into `CelerityResources`.
Test Plan: Reloaded a bunch of pages, rebuilt map.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7869
Summary: Ref T4222. Same deal as D7867, but for this other super nebulous "return a blob of stuff" method.
Test Plan: Regenerated map, browsed around, etc.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7868
Summary: Ref T4222. Currently, this exposes a bunch of information about the Celerity internals. This information is difficult to preserve exactly with the new maps. Strengthen the API by providing more specific capabilities.
Test Plan: Regenerated map, browsed around.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7867
Summary: Ref T4222. Continues porting `scripts/celerity_mapper.php` functionality into `bin/celerity map`. This is pretty much a `1:1` port with no functional changes, but hopefully the code is a little better factored. Particularly, more responsibilities are pluggalbe through `CelerityResources` now.
Test Plan: Ran `bin/celerity map` and inexpected the `var_dump()` output, which appeared to make sense.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7865
Summary:
Ref T4222. Moves us toward a more modern Celerity CLI, and moves map discovery into the classtree. This is a little bit bulky (and means you can't ship completely standalone celerity maps) but has the advantage of being completely standard, and we could subclass maps into an auto-discovering map later if we have a need for it.
This doesn't affect the existing Celerity stuff. I'm going to build the new stuff in parallel, and then swap us over at the end.
Test Plan: Ran `bin/celerity map`, got reasonable-looking output.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7864
Summary: Ref T4222. This was used by Facebook while developing Releeph, but should no longer be necessary since Releeph is in the upstream. I can't get an answer out of Facebook about whether they still use it or not (see T4227), so nuke it. We're going to replace it with a more general mechanism (see T4222).
Test Plan: Regenerated celerity map. Browsed some pages, still got resources.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7863
Summary:
Ref T4264. This gets most of the plumbing in for "object" rules, which will bind to a specific object, like a repository or project.
It does not yet let you actually create these rules.
Test Plan: Ran `storage upgrade`, created/edited rules, browsed Herald.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7847
Summary:
Ref T2015. Not directly related to Drydock, but I've wanted to do this for a bit.
Introduce a common base class for all the workflows in the scripts in `bin/*`. This slightly reduces code duplication by moving `isExecutable()` to the base, but also provides `getViewer()`. This is a little nicer than `PhabricatorUser::getOmnipotentUser()` and gives us a layer of indirection if we ever want to introduce more general viewer mechanisms in scripts.
Test Plan: Lint; ran some of the scripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7838
Summary:
Ref T2015. Currently, Drydock has a `wait-for-lease` workflow which is invoked in the background by the `lease` workflow.
The goal of this mechanism is to allow `bin/drydock lease` to print out logs as the lease is acquired. However, this predates the `runAllTasksInProcess` flags, and they provide a simpler and more robust way (potentially with `--trace` and `PhutilConsole`) to do synchronous execution and debug logging.
Simplify this whole mechanism: just run everything in-process in `bin/drydock lease`, and do logging via `--trace`. We could thread a `PhutilConsole` through things too, but this seems good enough for now.
Also various cleanup/etc.
Test Plan: Ran `bin/drydock lease`. Ran `bin/harbormaster build X --plan Y`, for `Y` being a Drydock-dependent build plan.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7835
Summary:
Ref T1049. Generally, it's useful to separate test/trial/manual runs from production/automatic runs.
For example, you don't want to email a bunch of people that the build is broken just because you messed something up when writing a new build plan. You'd rather try it first, then promote it into production once you have some good runs.
Similarly, test runs generally should not affect the outside world, etc. Finally, some build steps (like "wait for other buildables") may want to behave differently when run in production/automation than when run in a testing environment (where they should probably continue immediately).
So, formalize the distinction between automatic buildables (those created passively by the system in response to events) and manual buildables (those created explicitly by users). Add filtering, and stop the automated parts of the system from interacting with the manual parts (for example, we won't show manual results on revisions).
This also moves the "Apply Build Plan" to a third, new home: instead of the sidebar or Buildables, it's now on plans. I think this generally makes more sense given how things have developed. Broadly, this improves isolation of test environments.
Test Plan: Created some builds, browsed around, used filters, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7824
Summary:
Fixes two issues:
- When rendering a task's details, we currently issue a policy-oblivious query. Instead, issue a policy-aware query.
- The formatting is a little bit weird, with the top half in a box and the bottom half with an older style. Make them consistent.
Test Plan: Looked at the detail pages for several tasks in queue.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7812
Summary: Ref T4010. I'll hold this for a bit, but we should eventually drop this table once the dust has settled.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7372
Summary: Ref T4195. We need these in Herald, since HeraldTranscripts need to refer to a PHID which they acted upon.
Test Plan:
Ran migration, got PHIDs:
mysql> select phid from repository_pushlog limit 3;
+--------------------------------+
| phid |
+--------------------------------+
| PHID-PSHL-25jnc6cjgzw5rwqgmr7r |
| PHID-PSHL-2vrvmtslkrj5yv7nxsv2 |
| PHID-PSHL-34x262zkrwoka6mplony |
+--------------------------------+
3 rows in set (0.00 sec)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7780
Summary: Fixes T4241. Ref T4206. See T4241 for a description here. Generally, when we connect a fat pipe (`git-upload-pack`) to a narrow one (`git` over SSH) we currently read limitless data into memory. Instead, throttle reads until writes catch up. This is now possible because of the previous changes in this sequence.
Test Plan:
- Ran `git clone` and `git push` on the entire Wine repository.
- Observed CPU and memory usage.
- Memory usage was constant and low, CPU usage was high only during I/O (which is expected, since we have to actually do work, although thre might be room to further reduce this).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4241, T4206
Differential Revision: https://secure.phabricator.com/D7776
Summary: Ref T4189. Updates the Phabricator stuff to use the new, more sensible semantics from D7769. Basically, this works correctly now and doesn't need workarounds.
Test Plan: Pushed Wine repo in 1m13s.
Reviewers: btrahan, zeeg
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7770
Summary: This implements support for enforcing and setting policies in Phragment.
Test Plan: Set policies and ensured they were enforced successfully.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7751
Summary:
Ref T1049. See discussion in D7745. We have some specific interest in this for D7745, but generally we want to consume tasks with expired leases in roughly FIFO order, just like we consume new tasks in roughly FIFO order. Currently, when we select an expired task we order them by `id`, but this is the original insert order, not lease expiration order. Instead, order by `leaseExpires`.
This query is actually much better than the old one was, since the WHERE part is `leaseExpries < VALUE`.
Test Plan: Ran `EXPLAIN` on the query. Ran a taskmaster in debug mode and saw it lease new and expired tasks successfully.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7746
Summary:
Ref T4212. This implements snapshots in Phragment, which allows you to take a snapshot of a fragment at a given point in time, and download a ZIP of the snapshot as it was in this state.
There's also functionality for deleting and promoting snapshots. You can promote a snapshot to either the latest version or any other snapshot of the fragment.
Test Plan: Clicked around, took some snapshots, promoted them to different points and deleted snapshots. Also downloaded ZIPs of the snapshots and saw the right versions coming through for all the files downloaded.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205, T4212
Differential Revision: https://secure.phabricator.com/D7741
Summary:
This implements support for creating and updating fragments from ZIP files. It allows you to upload a ZIP via the Files application, create a fragment from it, and have it recursively imported into Phragment. Updating that folder with another ZIP will recursively create, update and delete files as appropriate.
The logic for creating and updating fragments from files has also been centralized into the PhragmentFragment class. Directories are also now supported; a directory fragment is simply a fragment that has no patches; thus a directory fragment can be converted to a file fragment by uploading a first patch for it.
Test Plan: Uploaded ZIP files through the interface and saw all of the fragments get created and updated as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7729
Summary: Ref T4205. This is an initial implementation of Phragment. You can create and browse fragments in the system (but you can't yet view a fragment's patches / history).
Test Plan: Clicked around and created fragments.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7726
Summary:
If you do something like this:
// Missing $user->getPHID()!
$object->setUserPHID($user)->save();
...you get a very unhelpful exception:
Expected a scalar or null for %s conversion. Query: %s
This doesn't give you any hints about what's wrong. Instead, provide a more useful exception:
Unable to insert or update object of class DifferentialRevision, field 'title' has a nonscalar value.
Test Plan: {F87614}
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7725
Summary: This adds a handful of 'Main Header' colors to change the look of Phabricator very slightly. I know I would probably set my dev header to a different color.
Test Plan: Tested each css class and color, can add more in the future.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7731
Summary: This implements support for explicitly marking the sequence of build steps. Users can now drag and re-order build steps in plans, and artifact dependencies are re-calculated so that if you move "Run Command" before "Lease Host", the "Run Command" step has it's artifact setting cleared and thus the step becomes invalid.
Test Plan: Re-ordered build steps and observed dependencies being correctly recalculated.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7715
Summary:
`PhabricatorPolicyFilter` has a bug right now where it lets through objects incorrectly if:
- the query requests two or more policies;
- the object satisfies at least one of those policies; and
- policy exceptions are not enabled.
This would be bad, but there's only one call in the codebase which satisfies all of these conditions, in the Maniphest batch editor. And it's moot anyway because edit operations get another policy check slightly later. So there is no policy/security impact from this flaw.
(The next diff relies on this behavior, which is how I caught it.)
Test Plan:
- Added a failing unit test and made it pass.
- Grepped the codebase for `requireCapabilities()` and verified that there is no security impact. Basically, 99% of callsites use `executeOne()`, which throws anyway and moots the filtering.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7721
Summary:
Ref T4195. This log serves two purposes:
- It's a log, so you can see what happened. Particularly, in Git/Hg, there is no other way to tell:
- Who //pushed// a change (vs committed / authored)?
- When was a change pushed?
- What was the old value of some tag/branch before someone destroyed it?
- We can hand these objects off to Herald to implement pre-commit rules.
This is a very basic implementation, but gets some data written and has a basic UI for it.
Test Plan: {F87339}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7705
Summary: This implements build targets as outlined in D7582. Build targets represent an instance of a build step particular to the build. Logs and artifacts have been adjusted to attach to build targets instead of build / build step pairs.
Test Plan: Ran builds and clicked around the interface. Everything seemed to work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T1049
Differential Revision: https://secure.phabricator.com/D7703
Summary:
//(this diff used to be about applying policies to blueprints)//
This restructures Drydock so that blueprints are instances in the DB, with an associated implementation class. Thus resources now have a `blueprintPHID` instead of `blueprintClass` and DrydockBlueprint becomes a DAO. The old DrydockBlueprint is renamed to DrydockBlueprintImplementation, and the DrydockBlueprint DAO has a `blueprintClass` column on it.
This now just implements CAN_VIEW and CAN_EDIT policies for blueprints, although they are probably not enforced in all of the places they could be.
Test Plan: Used the `create-resource` and `lease` commands. Closed resources and leases in the UI. Clicked around the new and old lists to make sure everything is still working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T2015
Differential Revision: https://secure.phabricator.com/D7638
Summary: Fixes T4149. This could be a little cleaner (configurable time limits, explicit timeout errors) but stop the major case of looping/infinite commands.
Test Plan: Added `sleep 5 &&` and set timeout to 1, saw an error + kill.
Reviewers: btrahan, skyronic
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4149
Differential Revision: https://secure.phabricator.com/D7651
Summary:
Ref T4038. This adds everything except the actual pushing part for mirrors.
This isn't the most beautiful or sophisticated UI, but I want get the authoritative repositories self-hosted and get users beta-ing hosting as soon as possible. We can do transactions, etc., later on.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4038
Differential Revision: https://secure.phabricator.com/D7632
Summary: Fixes T4122. Ref T2230. Instead of storing credentials on each repository, store them in Passphrase. This allows easy creation/management of many repositories which share credentials.
Test Plan:
- Upgraded repositories.
- Created and edited repositories.
- Pulled HTTP and SSH repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230, T4122
Differential Revision: https://secure.phabricator.com/D7629
Summary: Ref T4122. Add an edge to keep track of where a credential is used, and show it in the UI.
Test Plan:
See "Used By":
{F84099}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7628
Summary: ...and get the basic edit flow "working" for a new NuanceSourceDefinition - the Phabricator Form. ...and fix a dumb bug in the query class so when you redirect to the view page / try to edit an existing NuanceSource you don't fatal.
Test Plan: played around with the edit form and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7585
Summary:
Ref T4122. Implements a credential management application for the uses described in T4122.
@chad, this needs an icon, HA HA HAHA HA BWW HA HA HA
bwahaha
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7608
Summary:
We've been having trouble with viewing diffs timing out when there's a lot of unit test failures. It was caused by formatting userdata for every single failure. The expensive part of this was actually creating the engine for every result, so moved the construction outside of the loop.
Diffs that timed out (2 min) loading before load in around 6 seconds now.
Test Plan: Loaded diffs that used to time out. Verified that details still looked right when Show Full Unit Test Results Is Clicked.
Reviewers: epriestley, keegancsmith, lifeihuang, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, andrewjcg
Differential Revision: https://secure.phabricator.com/D7581
Summary:
Ref T4110. This denormalized field used to power "Group By: Assigned" got dropped in the T2217 migration at some point.
Restore its population, and fix all the data in the database.
Test Plan: Ran migration, verified database came out reasonable-looking. Reassigned a task, verified database. Ran a "Group By: assigned" query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4110
Differential Revision: https://secure.phabricator.com/D7602
Summary:
Ref T2230. When fully set up, we have up to three users who all need to write into the repositories:
- The webserver needs to write for HTTP receives.
- The SSH user needs to write for SSH receives.
- The daemons need to write for "git fetch", "git clone", etc.
These three users don't need to be different, but in practice they are often not likely to all be the same user. If for no other reason, making them all the same user requires you to "git clone httpd@host.com", and installs are likely to prefer "git clone git@host.com".
Using three different users also allows better privilege separation. Particularly, the daemon user can be the //only// user with write access to the repositories. The webserver and SSH user can accomplish their writes through `sudo`, with a whitelisted set of commands. This means that even if you compromise the `ssh` user, you need to find a way to escallate from there to the daemon user in order to, e.g., write arbitrary stuff into the repository or bypass commit hooks.
This lays some of the groundwork for a highly-separated configuration where the SSH and HTTP users have the fewest privileges possible and use `sudo` to interact with repositories. Some future work which might make sense:
- Make `bin/phd` respect this (require start as the right user, or as root and drop privileges, if this configuration is set).
- Execute all `git/hg/svn` commands via sudo?
Users aren't expected to configure this yet so I haven't written any documentation.
Test Plan:
Added an SSH user ("dweller") and gave it sudo by adding this to `/etc/sudoers`:
dweller ALL=(epriestley) SETENV: NOPASSWD: /usr/bin/git-upload-pack, /usr/bin/git-receive-pack
Then I ran git pushes and pulls over SSH via "dweller@localhost". They successfully interacted with the repository on disk as the "epriestley" user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7589
Summary:
- Add an option for the queue.
- By default, enable it.
- Dump new users into the queue.
- Send admins an email to approve them.
Test Plan:
- Registered new accounts with queue on and off.
- As an admin, approved accounts and disabled the queue from email.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7576
Summary:
- If you're an administrator and there are users waiting for approval, show a count on the home page.
- Sort out the `isUserActivated()` access check.
- Hide all the menu widgets except "Logout" for disabled and unapproved users.
- Add a "Log In" item.
- Add a bunch of unit tests.
Test Plan: Ran unit tests, clicked around as unapproved/approved/logged-in/logged-out users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D7574
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:
- Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
- Migrate all the existing users.
- When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
- Just make the checks look at the `isEmailVerified` field.
- Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
- Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
- When the queue is enabled, registering users are created with `isApproved = false`.
- Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
- They go to the web UI and approve the user.
- Manually-created accounts are auto-approved.
- The email will have instructions for disabling the queue.
I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.
Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.
Test Plan:
- Ran migration, verified `isEmailVerified` populated correctly.
- Created a new user, checked DB for verified (not verified).
- Verified, checked DB (now verified).
- Used Conduit, People, Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7572
Summary:
Ref T2230. The SVN protocol has a sensible protocol format with a good spec here:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/libsvn_ra_svn/protocol
Particularly, compare this statement to the clown show that is the Mercurial wire protocol:
> It is possible to parse an item without knowing its type in advance.
WHAT A REASONABLE STATEMENT TO BE ABLE TO MAKE ABOUT A WIRE PROTOCOL
Although it makes substantially more sense than Mercurial, it's much heavier-weight than the Git or Mercurial protocols, since it isn't distributed.
It's also not possible to figure out if a request is a write request (or even which repository it is against) without proxying some of the protocol frames. Finally, several protocol commands embed repository URLs, and we need to reach into the protocol and translate them.
Test Plan: Ran various SVN commands over SSH (`svn log`, `svn up`, `svn commit`, etc).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7556
Summary:
Ref T2230. This is substantially more complicated than Git, but mostly because Mercurial's protocol is a like 50 ad-hoc extensions cobbled together. Because we must decode protocol frames in order to determine if a request is read or write, 90% of this is implementing a stream parser for the protocol.
Mercurial's own parser is simpler, but relies on blocking reads. Since we don't even have methods for blocking reads right now and keeping the whole thing non-blocking is conceptually better, I made the parser nonblocking. It ends up being a lot of stuff. I made an effort to cover it reasonably well with unit tests, and to make sure we fail closed (i.e., reject requests) if there are any parts of the protocol I got wrong.
A lot of the complexity is sharable with the HTTP stuff, so it ends up being not-so-bad, just very hard to verify by inspection as clearly correct.
Test Plan:
- Ran `hg clone` over SSH.
- Ran `hg fetch` over SSH.
- Ran `hg push` over SSH, to a read-only repo (error) and a read-write repo (success).
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7553
Summary:
Ref T2230. In Git, we can determine if a command is read-only or read/write from the command itself, but this isn't the case in Mercurial or SVN.
For Mercurial and SVN, we need to proxy the protocol that's coming over the wire, look at each request from the client, and then check if it's a read or a write. To support this, provide a more flexible version of `passthruIO`.
The way this will work is:
- The SSH IO channel is wrapped in a `ProtocolChannel` which can parse the the incoming stream into message objects.
- The `willWriteCallback` will look at those messages and determine if they're reads or writes.
- If they're writes, it will check for write permission.
- If we're good to go, the message object is converted back into a byte stream and handed to the underlying command.
Test Plan: Executed `git clone`, `git clone --depth 3`, `git push` (against no-write repo, got error), `git push` (against valid repo).
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, asherkin, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7551
Test Plan: This is one of the rare moments where unit tests for views would be useful.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7547
Summary:
Depends on D7519.
This implements support for build logs in Harbormaster. This includes support for appending to a log from the "Run Remote Command" build step.
It also adds the ability to cancel builds.
Currently the build view page doesn't update the logs live; I'm sure this can be achieved with Javelin, but I don't have enough experience with Javelin to actually make it poll from updates to content in the background.
{F79151}
{F79153}
{F79150}
{F79152}
Test Plan:
Tested this by setting up SSH on a Windows machine and using a Remote Command configured with:
```
C:\Windows\system32\cmd.exe /C cd C:\Build && mkdir Build_${timestamp} && cd Build_${timestamp} && git clone --recursive https://github.com/hach-que/Tychaia.git && cd Tychaia && Protobuild.exe && C:\Windows\Microsoft.NET\Framework\v4.0.30319\MSBuild.exe Tychaia.Windows.sln
```
and observed the output of the build stream from the Windows machine into Phabricator.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7521
Summary:
I updated the wiki too - https://secure.phabricator.com/w/projects/pebkac/ - with what I am thinking right now. Rough plan here is
- next diff:
- implement editors and transactions
- implement "web type" for contact source
- /pebkac/item/new/ will be the entry point for this
- implement "actions" on a contact
- probably some "polish" on the scaffolding laid out here; like "create" permissions maybs
- diffs after that:
- implement "twitter" type for source
- implement email reply handler stuff for item and source
Probs a great time to blast huge holes in all this stuff. :D
Test Plan: these pages load and arc lint doesn't complain
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7465
Summary:
Ref T1049. Ref T2222. `DifferentialDiff` does not currently have a PHID, but we need it for Harbormaster and ApplicationTransactions. See some discussion in D7501.
(I split the SQL into two sections so we can't fail in the middle. At some point, I'd like to do a pass on the migration stuff and get this happening automatically, and also simplify the PatchList.)
Test Plan:
- Ran `bin/storage upgrade`.
- Checked for valid PHIDs in the database.
- Used `phid.query` to look up a diff by PHID.
- Created a new diff and verified it got a PHID.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2222, T1049
Differential Revision: https://secure.phabricator.com/D7513
Summary:
Depends on D7498.
This implements support for a "build step implementation". Build steps have an associated class name (which makes the class in PHP) and a details field, which is serialized JSON (same as PhabricatorRepository).
This also implements a SleepBuildStepImplementation which just pauses the build for a specified period of seconds.
Test Plan:
Inserted a build step with `insert into harbormaster_buildstep (phid, buildPlanPHID, className, details, dateCreated, dateModified) values ('', 'PHID-HMCP-zkh5w6czfbfpk2gxwdeo', 'SleepBuildStepImplementation', '{"seconds":5}', NOW(), NOW());` (adjusting the build plan PHID as appropriate).
Started the daemon and applied the build plan to a buildable, and saw the daemon take a 5 second delay after creating `SleepBuildStepImplementation`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7499
Summary: The idea is to have all `phtize` definitions in applications to allow their separation.
Test Plan: Clicked View Options after mangling the translation.
Reviewers: epriestley
Reviewed By: epriestley
CC: btrahan, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7345
Summary: This allows users to set their HTTP access passwords via Diffusion interface.
Test Plan: Clicked the "Set HTTP Access Password" link, set a password and saw it appear in the DB.
Reviewers: #blessed_reviewers, hach-que, btrahan
Reviewed By: hach-que
CC: Korvin, epriestley, aran, jamesr
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7462
Summary:
`RepositoryStatusMessage` is basically a key/value table associated with a repository that I'm using to let the daemons store the most recent event of a given type, so we can easily show it on the status dashboard. I think this will be a lot easier for users to figure out than digging through logfiles.
I'm also going to write the "this needs a pull" status here eventually, for reducing the time lapse between pushes and discovery.
- Add storage for these messages.
- Have the pull engine populate the INIT phase. I'll do the FETCH phase next.
- Update the status readout to show all the various states.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7461
Summary:
Fixes T3416. Fixes T1733.
- Adds a flag to the commit table showing whether or not we have parsed it.
- The flag is set to `0` initially when the commit is discovered.
- The flag is set to `1` when the changes are parsed.
- The UI can now use the flag to distinguish between "empty commit" and "commit which we haven't imported changes for yet".
- Simplify rendering code a little bit.
- Fix an issue with the Message parser for empty commits.
- There's a key on the flag so we can do `SELECT * FROM repository_commit WHERE repositoryID = %d AND importStatus = 0 LIMIT 1` soon, to determine if a repository is fully imported or not. This will let us improve the UI (Ref T776, Ref T3217).
Test Plan:
- Ran `bin/storage upgrade -f`.
- Created an empty commit.
- Without the daemons running, ran `bin/repository pull GTEST` and `bin/repository discover GTEST`.
- Viewed web UI to get the first screenshot ("Still Importing...").
- Ran the message and change steps with `scripts/repository/reparse.php`.
- Viewed web UI to get the second screenshot ("Empty Commit").
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T776, T1733, T3416, T3217
Differential Revision: https://secure.phabricator.com/D7428
Summary: Like D7423, but for SSH.
Test Plan: Ran `git clone ssh://...`, got a clone.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7424
Summary:
- Add web UI for configuring SSH hosting.
- Route git reads (`git-upload-pack` over SSH).
Test Plan:
>>> orbital ~ $ git clone ssh://127.0.0.1/
Cloning into '127.0.0.1'...
Exception: Unrecognized repository path "/". Expected a path like "/diffusion/X/".
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
>>> orbital ~ $ git clone ssh://127.0.0.1/diffusion/X/
Cloning into 'X'...
Exception: No repository "X" exists!
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
>>> orbital ~ $ git clone ssh://127.0.0.1/diffusion/MT/
Cloning into 'MT'...
Exception: This repository is not available over SSH.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
>>> orbital ~ $ git clone ssh://127.0.0.1/diffusion/P/
Cloning into 'P'...
Exception: TODO: Implement serve over SSH.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7421
Summary: No editing or view yet, just adds the schema and a policy default. Part of D7391.
Test Plan: `bin/storage upgrade`
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7415
Summary:
Gets rid of some old Differential-specific nonsense and replaces it with general runtime-pluggable Remarkup rules.
Facebook: This removes two options which may be in use. Have any classes being added via config here just subclass the new abstract bases instead. This should take 5 seconds to fix. You can adjust order by overriding `getPriority()` on the rules, if necessary.
Test Plan: See comments.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, andrewjcg, aran
Differential Revision: https://secure.phabricator.com/D7393
Summary: Makes a white hover icon show on the policy dropdown. Also fixed some spacing. Fixes T4017
Test Plan: hover over the policy dropdown
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4017
Differential Revision: https://secure.phabricator.com/D7388
Summary:
Ref T1049. I don't really want to sink too much time into this right now, but a seemingly reasonable architecture came to me in a dream. Here's a high-level overview of how things fit together:
- **"Build"**: In Harbormaster, "build" means any process we want to run against a working copy. It might actually be building an executable, but it might also be running lint, running unit tests, generating documentation, generating symbols, running a deploy, setting up a sandcastle, etc.
- `HarbormasterBuildable`: A "buildable" is some piece of code which build operations can run on. Generally, this is either a Differential diff or a Diffusion commit. The Buildable class just wraps those objects and provides a layer of abstraction. Currently, you can manually create a buildable from a commit. In the future, this will be done automatically.
- `HarbormasterBuildStep`: A "build step" is an individual build operation, like "run lint", "run unit", "build docs", etc. The step defines how to perform the operation (for example, "run unit tests by executing 'arc unit'"). In this diff, this barely exists.
- `HarbormasterBuildPlan`: This glues together build steps into groups or sequences. For example, you might want to "run unit", and then "deploy" if the tests pass. You can create a build plan which says "run step "unit tests", then run step "deploy" on success" or whatever. In the future, these will also contain triggers/conditions ("Automatically run this build plan against every commit") and probably be able to define failure actions ("If this plan fails, send someone an email"). Because build plans will run commands, only administrators can manage them.
- `HarbormasterBuild`: This is the concrete result of running a `BuildPlan` against a `Buildable`. It tracks the build status and collects results, so you can see if the build is running/successful/failed. A `Buildable` may have several `Build`s, because you can execute more than one `BuildPlan` against it. For example, you might have a "documentation" build plan which you run continuously against HEAD, but a "unit" build plan which you want to run against every commit.
- `HarbormasterBuildTarget`: This is the concrete result of running a `BuildStep` against a `Buildable`. These are children of `Build`. A step might be able to produce multiple targets, but generally this is something like "Unit Tests" or "Lint" and has an overall status, so you can see at a glance that unit tests were fine but lint had some issues.
- `HarbormasterBuildItem`: An optional subitem for a target. For lint, this might be an individual file. For unit tests, an individual test. For normal builds, an executable. For deploys, a server. For documentation generation, there might just not be subitems.
- `HarbormasterBuildLog`: Provides extra information, like command/execution transcripts. This is where stdout/stderr will get dumped, and general details and other messages.
- `HarbormasterBuildArtifact`: Stores side effects or results from build steps. For example, something which builds a binary might put the binary in "Files" and then put its PHID here. Unit tests might put coverage information here. Generally, any build step which produces some high-level output object can use this table to record its existence.
This diff implements almost nothing and does nothing useful, but puts most of these object relationships in place. The two major things you can't easily do with these objects are:
1) Run arbitrary cron jobs. Jenkins does this, but it feels tacked on and I don't know of anyone using it for that. We could create fake Buildables to get a similar effect, but if we need to do this I'd rather do it elsewhere in general. Build and cron/service/monitoring feel like pretty different problems to me.
2) Run parameterized/matrix steps (maybe?). Bamboo has this plan/stage/task/job breakdown where a build step can generate a zillion actual jobs, like "build client on x86", "build server on x86", "build client on ARM", "build server on ARM", etc. We can sort of do this by having a Step map to multiple Targets, but I haven't really thought about it too much and it may end up being not-great. I'd guess we have like an 80% chance of getting a clean implementation if/when we get there. I suspect no one actually needs this, or when they do they'll just implement a custom Step and it can be parameterized at that level. I'm not too worried about this overall.
The major difference between this and Jenkins/Bamboo/TravisCI is that all three of those are **plan-centric**: the primary object in the system is a build plan, and the dashboard shows you all your build plans and the current status. I don't think this is the right model. One disadvantage is that you basically end up with top-level messaging that says "Trunk is broken", not "Trunk was broken by commit af32f392f". Harbormaster is **buildable-centric**: the primary object in the system is stuff you can run build operations against (commits/branches/revisions), and actual build plans are secondary. The main view will be "recent commits on this branch, and whether they're good or not" -- which I think is what's most important in a larger/more complex product -- not the pass/fail status of all jobs. This also makes it easier and more natural to integrate with Differential and Diffusion, which both care about the overall status of the commit/revision, not the current status of jobs.
Test Plan: Poked around, but this doesn't really do anything yet.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, chad, aran, seporaitis
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7368
Summary: Ref T4010. Adds a history page and restores the transaction title strings, which previously sort-of existed in the defunct feed story class.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7371
Summary:
Ref T4010. Projects have a weird proto-version of ApplicationTransactions which is very similar but not quite the same.
Move the storage to a modern format, but keep all the other code for now.
Test Plan: Migrated project transactions; edited projects.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7370
Summary:
Ref T1344. This is //very// rough. Some UI issues:
- Empty states for the board and columns are junky.
- Column widths are crazy. I think we need to set them to fixed-width, since we may have an arbitrarily large number of columns?
- I don't think we have the header UI elements in M10 yet and that mock is pretty old, so I sort of very roughly approximated it.
- What should we do when you click a task title? Popping the whole task in a dialog is possible but needs a bunch of work to actually work. Might need to build "sheets" or something.
- Icons are slightly clipped for some reason.
- All the backend stuff is totally faked.
Generally, my plan is just to use these to implement all of T390. Specifically:
- "Kanban" projects will have "Backlog" on the left. You'll drag them toward the right as you make progress.
- "Milestone" projects will have "No Milestone" on the left, then "Milestone 9", "Milestone 8", etc.
- "Sprint" projects will have "Backlog" on the left, then "Sprint 31", "Sprint 30", etc.
So all of these things end up being pretty much exactly the same, with some minor text changes and new columns showing up on the left vs the right or whatever.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: chad, aran, sascha-egerer
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7374
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.
This has several parts:
- For PolicyAware queries, provide an application class name method.
- If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
- For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.
Test Plan:
- Added a unit test to verify I got all the class names right.
- Browsed around, logged in/out as a normal user with public policies on and off.
- Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7367
Summary: Ref T4010. Adds storage and indexes for custom fields. These tables are the same as people/maniphest/differential.
Test Plan: Ran `bin/storage upgrade`.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7369
Summary:
Fixes T3675.
- Maniphest had a couple of old non-event listeners; move them to events.
- Make most of the similar listeners a little more similar.
- Add checks for access to the application.
Test Plan:
- Viewed profile, project, task, revision.
- Clicked all the actions.
- Blocked access to various applications and verified the actions vanished.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3675
Differential Revision: https://secure.phabricator.com/D7365
Summary:
Ref T3675. Some of these listeners shouldn't do their thing if the viewer doesn't have access to an application (for example, users without access to Differential should not be able to "Edit Tasks"). Set the stage for that:
- Introduce `PhabricatorEventListener`, which has an application.
- Populate this for event listeners installed by applications.
- Rename the "PeopleMenu" listeners to "ActionMenu" listeners, which better describes their modern behavior.
This doesn't actually change any behaviors.
Test Plan: Viewed Maniphest, Differntial, People.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3675
Differential Revision: https://secure.phabricator.com/D7364
Summary:
Fixes T2146. This is a really simple approach, you just do:
!print .rule {
whatever: blah;
}
And it transforms it into:
.printable .rule {
whatever: blah;
}
@media print {
.rule {
whatever: blah;
}
}
So we end up with these rules twice, but they should compress well and we shouldn't need that many of them, and this fix is way way simpler than all the nonsense I discussed in T2146.
Test Plan:
- Added a unit test.
- Added a simple rule to throw away the menubar when printing.
- Checked the latter with `/?__print__=1`.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T2146
Differential Revision: https://secure.phabricator.com/D7363
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.
The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.
Two risks here:
- I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
- This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.
I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.
Test Plan:
- Before migrating, then after migrating:
- Made a bunch of inlines (drafts, submitted).
- Edited and deleted inlines.
- Verified inlines showed up in preview.
- Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
- Verified that inlines ARE indexed when they're not drafts.
- Verified that drafts inlines make revisions appear as "with draft" in the revision list.
- Made left, right, and draft inlines.
- Migrated (`bin/storage upgrade`).
- Verified that my inlines from before the migration still showed up.
- (Repeated all the stuff above.)
- Manually inspected the inline comment table.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7139
Summary: This data was migrated by D6977 and is now obsolete. I'll hold this patch for a week or two in case we get reports of migration errors.
Test Plan: Ran storage upgrade, saw the table vanish. Grepped for references to the table.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6997
Summary: Believe it or not, I forgot how to create a link in Remarkup.
Test Plan: Clicked on it with selected URL, selected text and without a selection.
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7336
Summary:
Ref T603. Currently, we render handles the user doesn't have permission to see in a manner identical to handles that don't exist. This is confusing, and not required by policies (which restrict content, but permit knowledge that an object exists).
Instead, render them in different styles. Bad/invalid objects look like:
Unknown Object (Task)
Restricted objects look like:
[o] Restricted Task
...where `[o]` is the padlock icon.
Test Plan:
{F71100}
{F71101}
It's possible this renders weird somewhere, but I wasn't immediately able to find any issues. Yell if you see something.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7334
Summary: Makes it easy to choose distinctive icons for projects.
Test Plan:
{F71018}
{F71020}
{F71019}
{F71021}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7333
Test Plan: Translated 'bold text' as 'txt', clicked on B without selection, saw 'txt'.
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D7335
Summary: Ref T603. Give countdowns proper UI-level policy controls, and an application-level default policy. Put policy information in the header.
Test Plan:
- Adjusted default policy.
- Created new countdowns.
- Edited countdowns.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7322
Summary:
Ref T603. Several issues here:
1. Currently, `FileQuery` does not actually respect object attachment edges when doing policy checks. Everything else works fine, but this was missing an `array_keys()`.
2. Once that's fixed, we hit a bunch of recursion issues. For example, when loading a User we load the profile picture, and then that loads the User, and that loads the profile picture, etc.
3. Introduce a "Query Workspace", which holds objects we know we've loaded and know we can see but haven't finished filtering and/or attaching data to. This allows subqueries to look up objects instead of querying for them.
- We can probably generalize this a bit to make a few other queries more efficient. Pholio currently has a similar (but less general) "mock cache". However, it's keyed by ID instead of PHID so it's not easy to reuse this right now.
This is a bit complex for the problem being solved, but I think it's the cleanest approach and I believe the primitive will be useful in the future.
Test Plan: Looked at pastes, macros, mocks and projects as a logged-in and logged-out user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7309
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.
Some policy objects don't have real PHIDs:
PhabricatorTokenGiven
PhabricatorSavedQuery
PhabricatorNamedQuery
PhrequentUserTime
PhabricatorFlag
PhabricatorDaemonLog
PhabricatorConduitMethodCallLog
ConduitAPIMethod
PhabricatorChatLogEvent
PhabricatorChatLogChannel
Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.
Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.
Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7306
Summary: A set of random icons for use as project identifiers. 42, white.
Test Plan: photoshop, epriestley
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7290
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.
Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7283
Summary:
Ref T603. Ref T1279. Further improves transaction and policy support for Herald.
- Instead of deleting rules (which wipes out history and can't be undone) allow them to be disabled.
- Track disables with transactions.
- Gate disables with policy controls.
- Show policy and status information in the headers.
- Show transaction history on rule detail screens.
- Remove the delete controller.
- Support disabled queries in the ApplicationSearch.
Test Plan:
- Enabled and disabled rules.
- Searched for enabled/disabled rules.
- Verified disabled rules don't activate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279, T603
Differential Revision: https://secure.phabricator.com/D7247
Summary:
Ref T1279. No actual logical changes, but:
- You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
- You can now add projects as reviewers from the revision detail typeahead.
- You can now add projects as reviewers from the CLI (`#yoloswag`).
- Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).
I'll separate projects from users in the "Reviewers" tables in the next revision.
Test Plan:
- Added projects as reviewers using the web UI and CLI.
- Used `arc amend --show --revision Dnnn` to generate commit messages.
- Viewed revision with project reviewers in web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7230
Summary:
Ref T1279. This came to me in a dream.
The existing `differential_relationship` table has an `(objectPHID, type)` column, which theoretically is useful for queries like "revisions with X as a reviewer". In practice, I'm not sure it gets used much, but I can get it to show up in at least some query plans.
Add a similar index to the `edge` table. This sequences //before// D7227, which actually migrates the data.
Test Plan:
- Ran `bin/storage upgrade`.
- EXPLAIN'd a bunch of queries against different versions of the schema, this seemed helpful overall.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7232
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary:
Ref T603. I had to partially revert this earlier because it accidentally blocked access to Conduit and File data for installs without "policy.allow-public", since the applications are available to "all users" but some endpoints actually need to be available even when not logged in.
This readjusts the gating in the controller to properly apply application visibility restrictions, and then adds a giant pile of unit test coverage to make sure it sticks and all the weird cases are covered.
Test Plan:
- Added and executed unit tests.
- Executed most of the tests manually, by using logged in / admin / public / disabled users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7211
Summary: Ref T603. Make this rule properly policy-aware, and extend from `PhabricatorRemarkupRuleObject`.
Test Plan:
- Embedded an image, tested all options (name, link, float, layout, size).
- Used lightbox to view several images.
- Embedded a text file, tested all options (name).
- Embedded audio, tested all options (loop, autoplay).
- Attached a file via comment to a task, verified edge was created.
- Attached a file via comment to a conpherence, verified edge was created.
- Viewed old files, verified remarkup version bump rendered them correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7192
Summary:
Ref T603. Principally, I want to implement the rule "when you upload a file to an object, users must be able to see the object in order to see the file", since I think this is strongly in line with user expectation. For example, if you attach a file to a Conpherence, it should only be visible to members of that thread.
This adds storage for policies, but doesn't do anything interesting with it yet.
Test Plan: Ran `bin/storage upgrade`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7175
Summary: This adds the 'PHUIObjectBox' to nearly every place that should get it. I need to comb through Diffusion a little more. I've left Differential mostly alone, but may decide to do it anyways this weekend. I'm sure I missed something else, but these are easy enough to update.
Test Plan: tested each new layout.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7162
Summary: Ref T3887. Implements storage and editors, but not the actual audio part.
Test Plan: Edited audio, audio behaviors of macros. Transactions and email looked good. Hit error cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7159
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.
- Provide as much detail as possible.
- Fix all the strings for i18n.
- Explain special rules to the user.
- Allow indirect policy filters to raise policy exceptions instead of 404s.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7151
Summary:
Ref T2222. This sequences //before// D7139 and sorts out keys on the table. In particular:
- There was a fairly silly `draft` key modeled after Pholio; drop it.
- Add a `revisionPHID` key. This is queried mostly-transitionally on the revision view screen.
- Add a `changesetID` key. This is queried by a bunch of interfaces that want more surgical results than `revisionPHID` provides.
- Add an `authorPHID, transactionPHID` key. This is queried on the list interface to find pending drafts.
- Add a `legacy` key. This is queried by the feed publisher.
Test Plan: Used the query analyzer to hit all (I think?) of the pages, saw keyed queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7140
Summary: Ref T603. Paves the way for policy controls.
Test Plan: Ran storage upgrade, bumbled around in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7133
Summary: Ref T603. Move to real Query classes.
Test Plan:
- Ran `phd debug pull X` (where `X` does not match a repository).
- Ran `phd debug pull Y` (where `Y` does match a repository).
- Ran `phd debug pull`.
- Ran `repository pull`.
- Ran `repository pull X`.
- Ran `repository pull Y`.
- Ran `repository discover`.
- Ran `repository delete`.
- Ran `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7137
Summary:
Ref T603. Make almost every task read policy-aware. Notable exceptions are:
- Edge editor -- this stuff is prescreened and should be moved to ApplicationTransactions eventually anyway.
- Search/attach stuff -- this stuff needs some general work. The actual list should be fine since you can't pull handles. There may be a very indirect hole here where you could attach an object you can't see (but do know the ID of) to an object you can see. Pretty fluff.
- The "Tasks" field in Differential will let you reference objects you can't see. Possibly this is desirable, in the case of commandeering revisions. Mostly, it was inconvenient to get a viewer (I think).
Test Plan:
- Called `maniphest.info`.
- Called `maniphest.update`.
- Batch edited tasks.
- Dragged and dropped tasks to change subpriority.
- Subscribed and unsubscribed from a task.
- Edited a task.
- Created a task.
- Created a task with a parent.
- Created a task with a template.
- Previewed a task update.
- Commented on a task.
- Added a dependency.
- Searched for "T33" in object search dialog.
- Created a branch "T33", ran `arc diff`, verified link.
- Pushed a commit with "Fixes T33", verified close.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7119
Summary: Ref T2217. Fixes T3866. We need to do a little more massaging before we can set strings as the value on datetime controls.
Test Plan: Set default to "1:23 AM", "2001/02/03".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217, T3866
Differential Revision: https://secure.phabricator.com/D7116
Summary: Ref T2217. Render feed stories like "alincoln updated T123" instead of "alincoln updated this task.". Fix up some more translations.
Test Plan: Looked at feed, saw something a bit more reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7108
Summary:
Fixes T3867. We currently show more empty custom field values on task detail pages than we should, for at least two reasons:
- `<select />` fields with an empty string option store `""`, but users reasonably expect this to mean "no value".
- Old fields may have stored empty strings, and migrated forward.
This fix generally aligns behavior with user expectations. We could get more extreme about not storing `""` in the database, but I think this is generally a less surpsing fix.
Test Plan: Made a select with a `"" : "None"` option, selected it, saw it vanish from task detail screen.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3867
Differential Revision: https://secure.phabricator.com/D7105
Summary:
Ref T2217. Fixes T3876. We incorrectly have a unique key on `(authorPHID, transactionPHID)`, which prevents saving multiple versions of a comment.
I'm not entirely sure why this exists. I think it came from Pholio (where it works for inlines, because it has an additional component, but maybe should be adjusted) but we might need to wipe it out of more apps too.
Test Plan: Edited a comment in Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217, T3876
Differential Revision: https://secure.phabricator.com/D7102
Summary: Ref T2217. Cleans up some of the "attached %d file(s)" stuff.
Test Plan: Generated some of these transactions and verified they render more naturally.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7096
Summary: Ref T2217. Cleans up the table names. Moves old data to `maniphest_transaction_legacy`. We'll drop that eventually once it's more clear that I didn't break the world.
Test Plan: Did reads/writes to/from these tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7094
Summary:
Ref T2217. Ship "Merge in Duplicates" through the new editor. The only notable thing here is `setContinueOnMissingFields()`.
The problem this solves is that if you add a custom field and mark it as required, all existing tasks are "invalid" since they don't have a value, and trying to edit them will raise an error like "Some Custom Field is required!". This is fine for normal edits via the UI, since the user can just select/provide a value, but surgical edits to specific fields should just ignore these errors. Add the ability to ignore these errors and use it on all the field-speific editors.
Test Plan: Merged duplicates, including "invalid" duplicates with missing fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7084
Summary: Ref T2217. Route transaction rendering through modern code. This just affects the detail page. Some rough edges but nothing significant.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7070
Summary:
Ref T2217. This is the risky, hard part; everything after this should be smooth sailing. This is //mostly// clean, except:
- The old format would opportunistically combine a comment with some other transaction type if it could. We no longer do that, so:
- When migrating, "edit" + "comment" transactions need to be split in two.
- When editing now, we should no longer combine these transaction types.
- These changes are pretty straightforward and low-impact.
- This migration promotes "auxiliary field" data to the new CustomField/StandardField format, so that's not a straight migration either. The formats are very similar, though.
Broadly, this takes the same attack that the auth migration did: proxy all the code through to the new storage. `ManiphestTransaction` is now just an API on top of `ManiphestTransactionPro`, which is the new storage format. The two formats are very similar, so this was mostly a straight copy from one table to the other.
Test Plan:
- Without performing the migration, made a bunch of edits and comments on tasks and verified the new code works correctly.
- Droped the test data and performed the migration.
- Looked at the resulting data for obvious discrepancies.
- Looked at a bunch of tasks and their transaction history.
- Used Conduit to pull transaction data.
- Edited task description and clicked "View Details" on transaction.
- Used batch editor.
- Made a bunch more edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7068
Summary: Ref T2217. Add the tables and comment class for the new stuff. Not used yet.
Test Plan: Ran storage upgrade, browsed Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7066
Summary: Fixes T3864. If you define a "bool" control but don't define a
corresponding "strings", we currently fatal when trying to `idx()` into
`null`.
Auditors: btrahan
Summary:
Ref T603. See inlines for an explanation. The case where I hit this was loading the "Pending Differential Revisions" panel in Diffusion when logged out, after making a repository public.
What happens is that we load 10 revisions (say, D1 .. D10) but the user can't see any of them. We then try to load the next 10, but since the pagination is ordered by date modified, we need to base the next query on the modified date of the last thing we loaded (D10). However, since we use the viewer's policies to load that cursor object, it fails to load, and then we just issue the same query over and over again, loading D1 .. D10 until we run out of execution time.
Test Plan: Interface now loads correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7059
Summary: Improves transaction rendering for custom fields and standard custom fields.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7054
Summary:
- Add some TODO'd keys.
- Add policy fields.
Test Plan: Viewed repositories; created a new repository and verified it got the right default policy settings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7056
Summary: We aren't setting the value for select fields correctly, so when
editing they always show the first value instead of the correct value.
Test Plan: Set task from Apple -> Banana and Carrot -> Potato, saved, hit
"Edit", saw Banana / Potato.
Auditors: btrahan
Summary: Ref T3794. Drop auxiliary field, use standard field.
Test Plan: Performed migration, field seemed to survive it intact. Edited and viewed tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3794
Differential Revision: https://secure.phabricator.com/D7036
Summary: Makes Maniphest look more like standard fields to make the migration easier.
Test Plan: Edited tasks and users with required and invalid fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7035
Summary: Final standard capability from Maniphest fields.
Test Plan: This isn't really reachable now since users always exist, but faked it and verified the field prefilled as expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7033
Summary:
Ref T418. This is fairly messy, but basically:
- Add a validation phase to TransactionEditor.
- Add a validation phase to CustomField.
- Bring it to StandardField.
- Add validation logic for the int field.
- Provide support in related classes.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D7028
Summary: Support captions, so this can replace the Maniphest version.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7020
Summary: Ref T418. This is the last of the Maniphest field types, although I have a few more capabilities/options to port over.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D7018
Summary: These end up a little weird with subclassing instead of `switch`, but some day we could alias them to one another or something I guess. If I'm feeling brave, I might get rid of the "user" variant when I migrate Maniphest custom field specs, and turn it into "users, limit = 1" or something like that.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7010
Summary: See previous revisions. As maniphest.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7009
Summary: See D7006, etc. This one's easy since exposing it to ApplicationSearch doesn't make much sense.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7008
Summary: See D7006, etc. Brings this from Maniphest.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7007
Summary:
Currently, `ManiphestAuxiliaryFieldDefaultSpecification` uses about a dozen giant `switch` statements to implement stadard field types (int, string, date, bool, select, user, remarkup, etc). This is:
- pretty gross;
- not extensible; and
- doesn't really let us share that much code.
I got about halfway through porting a similar implementation into StandardField but I wasn't thrilled with it. Subclass StandardField instead to implement custom field types.
Test Plan: Added an "int" custom field, verified it had integer semantics and indexed into the integer index.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7005
Summary: Ref T418. Moves data from the Maniphest-specific table to the general one. This patch is a bit gross, but mostly about getting the reads and writes aimed correctly. Future patches will clean things up.
Test Plan: Migrated data across formats. Verified it survied the migration. Viewed and edited tasks' custom fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D6999
Summary: Ref T418. Maniphest has an obsolete class-based field selector. Replace it with CustomField-based selectors, which use the nice config UI and are generally way easier to use.
Test Plan: Added custom fields; edited and viewed custom fields on tasks. Everything worked as expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D6998
Summary: Ref T418. Depends on D6992. This adds index and value storage for Maniphest custom fields.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D6995
Summary:
Ref T2625. Ref T3794. Ref T418. Ref T1703.
This is a more general version of D5278. It expands CustomField support to include real integration with ApplicationSearch.
Broadly, custom fields may elect to:
- build indicies when objects are updated;
- populate ApplicationSearch forms with new controls;
- read inputs entered into those controls out of the request; and
- apply constraints to search queries.
Some utility/helper stuff is provided to make this easier. This part could be cleaner, but seems reasonable for a first cut. In particular, the Query and SearchEngine must manually call all the hooks right now instead of everything happening magically. I think that's fine for the moment; they're pretty easy to get right.
Test Plan:
I added a new searchable "Company" field to People:
{F58229}
This also cleaned up the disable/reorder view a little bit:
{F58230}
As it did before, this field appears on the edit screen:
{F58231}
However, because it has `search`, it also appears on the search screen:
{F58232}
When queried, it returns the expected results:
{F58233}
And the actually good bit of all this is that the query can take advantage of indexes:
mysql> explain SELECT * FROM `user` user JOIN `user_customfieldstringindex` `appsearch_0` ON `appsearch_0`.objectPHID = user.phid AND `appsearch_0`.indexKey = 'mk3Ndy476ge6' AND `appsearch_0`.indexValue IN ('phacility') ORDER BY user.id DESC LIMIT 101;
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| 1 | SIMPLE | appsearch_0 | ref | key_join,key_find | key_find | 232 | const,const | 1 | Using where; Using temporary; Using filesort |
| 1 | SIMPLE | user | eq_ref | phid | phid | 194 | phabricator2_user.appsearch_0.objectPHID | 1 | |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
2 rows in set (0.00 sec)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T1703, T2625, T3794
Differential Revision: https://secure.phabricator.com/D6992
Summary: Ref T2625. EVERYONE LOVES MIGRATIONS!!!
Test Plan:
- Created and migrated a query with every field, verified results were preserved.
- Created and migrated a query using "noproject" and "upforgrabs" magic, verified results were preserved.
Here's the pre-migration "everything" query:
{F58110}
Here it is after migration:
{F58111}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6977
Summary:
Ref T2625. Depends on D6971. Maniphest is complicated to implement cursor paging for. Builds on D6971 to do so.
This is //almost// complete. Paging on projects and authors doesn't quite work, I'll clean that up shortly. Left some TODOs.
Test Plan: Set page size to `3`, paged forward and backward in a bunch of group/order modes. Results seemed to be as expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6972
Summary:
We currently have two giant messes for paging across multiple columns (usually because one column is not unique), and I'm about to add a third for Maniphest.
Provide a more structured way to build these `A > a OR (A = a AND B > b)` clauses.
Test Plan: Set page size to `2` for Differential and Diffusion and paged forward and backward with a bunch of different orders set. Pages worked as expected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6971
Summary:
Fixes T3821. Maybe. The existing code seemed to have a bug and actually return the //commit phid//. Judging by the function name this is not intended.
Also, sorry to step on toes here -- I thought no one was assigned and was curious about loadRelativeEdges and here we are...
Test Plan: lots of logic here as I have no idea how to use Releeph.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3821
Differential Revision: https://secure.phabricator.com/D6967
Summary: See discussion in D6955. This provides a table we can JOIN against to (effectively) "ORDER BY project name", populates it intially, and keeps it up to date as projects are edited.
Test Plan:
- Ran storage upgrade, verified projects populated into the table.
- Edited a project, verified its entry updated.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6957
Summary:
See discussion in D6955. Provide an event for applications and users to update secondary search indexes.
Facebook: I don't recall exactly how all the search stuff is rigged up, but this might provide a more practical / less fragile alternative. I think it publishes into ElasticSearch now, and then intern somehow handles the result merge at display time, implictly relying on Phabricator's storage format? A cleaner approach might be to publish a secondary "intern" index in a standard format.
Test Plan: Ran `bin/search index --type proj --trace`, saw events fire.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D6956
Summary: Depends on D6952. Unpunts there since I'm rolling into a swamp full of schema changes.
Test Plan: Issued date-constrained query and saw key as a candidate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6954
Summary: Noticed this in the schema. "Touches" were an idea that never really got off the ground, as we built out more/better notification channels instead. Essentially, they recorded any object you'd ever interacted with. Maybe this will be useful some day, but for now it does nothing and can't be interacted with. Nuke it.
Test Plan: `grep`, loaded Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6953
Summary: I'm going to just delete all this code at some point, but fixing it now means piles of single gets, so unbreak it first. I'll file something.
Test Plan: Releeph is less fataley.
Reviewers: andrewjcg, btrahan
Reviewed By: andrewjcg
CC: aran
Differential Revision: https://secure.phabricator.com/D6945
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))
Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, FacebookPOC
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6941
Summary: Ref T603. Ref D6941.
Test Plan: Clicked around all over - looked good. I plan to re-test D6941 to make sure the executeOne case works now as intended
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6944
Summary:
Ref T603. Moves to detangle and optimize how we apply policies to filtering objects. Notably:
- Add a short circuit for omnipotent users.
- When performing project filtering, do a stricter check for user membership. We don't actually care if the user can see the project or not according to other policy constraints, and checking if they can may be complicated.
- When performing project filtering, do a local check to see if we're filtering the project itself. This is a common case (a project editable by members of itself, for example) and we can skip queries when it is satisfied.
- Don't perform policy filtering in ObjectQuery. All the data it aggregates is already filtered correctly.
- Clean up a little bit of stuff in Feed.
Test Plan: Pages like the Maniphest task list and Project profile pages now issue dramatically fewer queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6931
Summary: Simplify rendering of the repository list. For inactive repositories, mark them disabled.
Test Plan: {F57615}
Reviewers: btrahan, rockybean
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6921
Summary: Adds plain support for object lists that just look like lists
Test Plan: review UIexamples and a number of other applications
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6922
Summary:
Ref T988.
- Render "Implements:" as tags, too.
- Minor CSS tweak to tags in property lists.
- Add a bunch of group patterns to the Phabricator book.
- Fix some stuff with how hashes are computed and cached.
- Minor tweak to reuse the Diviner engine for slightly improved performance.
Test Plan: Regenerated and looked at documentation.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3811, T988
Differential Revision: https://secure.phabricator.com/D6912
Summary: this ends up being a little weird since you can't actually edit files. Also, since we create files all sorts of ways, sometimes without even having a user, we don't bother logging transactions for those events. Fixes T3651. Turns out this work is important for T3612, which is a priority of mine to help get Pholio out the door.
Test Plan: left a comment on a file. it worked! use bin/mail to verify mail content looked correct.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, wez
Maniphest Tasks: T3651, T3612
Differential Revision: https://secure.phabricator.com/D6789
Summary: This adds a number of new styles for Diviner documentation. Not sure I've covered all the bases or wrote this in the most efficient manner, but passing it along now for early review before tightening everything up.
Test Plan: Review various class pages.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6888
Summary:
Ref T3687. This adds a field which allows you to link Differential Revisions to JIRA issues.
This is just about as basic as it can get, but gets the job done. The field enables itself if you have a JIRA auth provide. You enter JIRA issues in a comma-delimited format and it generates appropriate edges.
Nothing is pushed to the issues yet.
The only real rough part here is that if you commandeer a revision which is linked to issues you can't see, editing it is difficult via the CLI. This seems pretty much like a non-issue, but at some point we can let the field throw some kind of "RecoverableInvalidFieldException" which just warns the user. The "no reviewers, continue anyway?" prompt could then use that too.
Test Plan:
- Edited via web UI, tried valid/invalid edits, checked that edges showed up in the database, added/removed issues, clicked issue links.
- Edited via CLI, tried valid/invalid edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6879
Summary: Starting to roll out the standard colors and spacing to action list, headers, and property views. Also softened the grey borders a hex.
Test Plan: Review Maniphest and Differential on desktop and mobile. Felt the flow of standardization waft over me.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6869
Summary:
Ref T3599
Go through everything, grep a bit, replace some bits.
Test Plan: Navigate around a bit
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3599
Differential Revision: https://secure.phabricator.com/D6871
Summary: This adds standard 'blues' and start integration of standard colors for text, backgrounds, and borders.
Test Plan: sb
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6857
Summary: This adds a set of standard grey colors for use in shading objects and importance. I'll follow up and start implementing in another diff.
Test Plan: Color UI Examples, Adobe Kuler
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6853
Summary:
Fixes T3782. Two changes:
- Remove the "Chaos" mode, which wasn't as funny as I'd hoped and has had a good run.
- Fix "Order" (now "Fullscreen") mode in Conpherence. Best fix I could come up with is dropping the "position: fixed" on all parents while in the mode.
Test Plan: Used Fullscreen mode in Conpherence in Chrome.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3782
Differential Revision: https://secure.phabricator.com/D6844
Summary: Fixes T3486. I don't love how this looks -- maybe we could try different icons? Like white icons on a brighter red/yellow background?
Test Plan: {F56299}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3486
Differential Revision: https://secure.phabricator.com/D6833
Summary:
Ref T988. This brings the class/interface atomizer over. A lot of parts of this are still varying degrees of very-rough, but most of the data ends up in approximatley the right place.
ALSO: PROGRESS BARS
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6817
Summary:
This attempts some consistency in form layouts. Notably, they all now contain headers and are 16px off the sides and tops of pages. Also updated dialogs to the same look and feel. I think I got 98% of forms with this pass, but it's likely I missed some buried somewhere.
TODO: will take another pass as consolidating these colors and new gradients in another diff.
Test Plan: Played in my sandbox all week. Please play with it too and let me know how they feel.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6806
Summary: Ref T3663. Does what it says on the tin.
Test Plan: Ran `storage upgrade`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3663
Differential Revision: https://secure.phabricator.com/D6778
Summary:
Releeph branch lists in project views have a bunch of custom UI right now; give them more standard UI and ApplicationSearch.
This drops a small piece of functionality: we now show only a total open request count instead of a detailed enumeration of each request status. I assume this is reasonable (that is, the important piece is "is there something to do on this branch?"), but we can muck with it if the more detailed status is important.
Test Plan: {F54344}
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3656
Differential Revision: https://secure.phabricator.com/D6764
Summary: We currently die if an event listener throws when registering (e.g., because it is misconfigured), but this prevents you from running `bin/config` to fix it, which is a bit silly.
Test Plan: Created this revision with an invalid listener in config.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6761
Summary:
Ref T1703. Ref T3718. This introduces `PhabricatorCustomFieldAttachment`, which is just a fancy `array()`. The goal here is to simplify `PhabricatorCustomFieldInterface` as much as possible.
In particular, it can now use common infrastructure (`assertAttached()`) and is more difficult to get wrong.
Test Plan: Edited custom fields on profile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703, T3718
Differential Revision: https://secure.phabricator.com/D6752
Summary:
Ref T1703. Ref T3718. The `PhabricatorCustomFieldList` seems like a pretty good idea. Move more code into it to make it harder to get wrong.
Also the sequencing on old/new values for these transactions was a bit off; fix that up.
Test Plan: Edited standard and custom profile fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703, T3718
Differential Revision: https://secure.phabricator.com/D6751
Summary:
Fixes T3661. Ref T3718. This makes Releeph custom fields extend PhabricatorCustomField so we can start moving over other pieces of infrastructure (rendering, storage, etc) to run through the same pathways. It's roughly the minimum amount of work required to be able to move forward.
NOTE: This removes per-project custom field selectors. Fields are now configured for an entire install. My understanding is that Facebook does not use this feature, and modern field infrastructure has moved away from selectors.
Test Plan: Viewed and edited projects, branches, and requests in Releeph. Grepped for removed config. Grepped for `field_selector`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3661, T3718
Differential Revision: https://secure.phabricator.com/D6750
Summary:
Ref T1702. Ref T3718. There are a couple of things going on here:
**PhabricatorCustomFieldList**: I added `PhabricatorCustomFieldList`, which is just a convenience class for dealing with lists of fields. Often, current field code does something like this inline in a Controller:
foreach ($fields as $field) {
// do some junk
}
Often, that junk has some slightly subtle implications. Move all of it to `$list->doSomeJunk()` methods (like `appendFieldsToForm()`, `loadFieldsFromStorage()`) to reduce code duplication and prevent errors. This additionally moves an existing list-convenience method there, out of `PhabricatorPropertyListView`.
**PhabricatorUserConfiguredCustomFieldStorage**: Adds `PhabricatorUserConfiguredCustomFieldStorage` for storing custom field data (like "ICQ Handle", "Phone Number", "Desk", "Favorite Flower", etc).
**Configuration-Driven Custom Fields**: Previously, I was thinking about doing these with interfaces, but as I thought about it more I started to dislike that approach. Instead, I built proxies into `PhabricatorCustomField`. Basically, this means that fields (like a custom, configuration-driven "Favorite Flower" field) can just use some other Field to actually provide their implementation (like a "standard" field which knows how to render text areas). The previous approach would have involed subclasssing the "standard" field and implementing an interface, but that would mean that every application would have at least two "base" fields and generally just seemed bleh as I worked through it.
The cost of this approach is that we need a bunch of `proxy` junk in the base class, but that's a one-time cost and I think it simplifies all the implementations and makes them a lot less magical (e.g., all of the custom fields now extend the right base field classes).
**Fixed Some Bugs**: Some of this code hadn't really been run yet and had minor bugs.
Test Plan:
{F54240}
{F54241}
{F54242}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1702, T1703, T3718
Differential Revision: https://secure.phabricator.com/D6749
Summary: Ref T3656. Releeph denormalizes branch cut point identifiers into Branch objects, but this information isn't useful or used for sorting, filtering, or enforcing unique constraints. Instead, derive it via noramlized pathways from the `cutPointCommitPHID`.
Test Plan: Ran storage upgrade. Ran `releephwork.getbranch` and `releeph.getbranches`. Grepped for `cutPointCommitIdentifier`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3656
Differential Revision: https://secure.phabricator.com/D6636
Summary:
Fixes T3660. Releeph Projects currently have an unused one-to-one mapping to Phabricator projects. This isn't consistent with other applications and has no integrations or uses. Get rid of it.
NOTE: Waiting for signoff from @legneato on T3660 before pulling the trigger here.
Test Plan: Created and edited Releeph projects. Grepped for references to project ID; there are a dozen or so but they're all either Releeph projects or Arcanist projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3660
Differential Revision: https://secure.phabricator.com/D6635
Summary: Ref T3655. Depends on D6633. This removes the writes and the column.
Test Plan: Created a project, edited a project. Verified the table doesn't have any keys including this column.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3655
Differential Revision: https://secure.phabricator.com/D6634
Summary: Ref T2769. I'm planning to keep this pretty simple, but we have this ad-hoc edit log for rules already and some other mess that we can clean up.
Test Plan: No effect yet; see future changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6654
Summary: Ref T3684 for discussion. This could be cleaned up a bit (it would be nice to draw entropy once per request, for instance, and maybe respect CSRF_TOKEN_LENGTH more closely) but should effectively mitigate BREACH.
Test Plan: Submitted forms; submitted forms after mucking with CSRF and observed CSRF error. Verified that source now has "B@..." tokens.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3684
Differential Revision: https://secure.phabricator.com/D6686
Summary:
Ref T2852. Two issues:
- Embeds (`T12`, `{T12}`) have some handle issues because handles run afoul of visibility checks under some configs. Make handles unconditionally visible.
- Asana links don't render correctly into text mode. Give them a valid text mode rendering so they don't flip out.
Test Plan: Made comments with `T12` and `http://app.asana.com/...` and published them to Asana.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6696
Summary: Email replies and subscribers seem to go hand in hand so deploy both at once.
Test Plan: played around with bin/mail. Verified replies posted comments on the paste.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3650
Differential Revision: https://secure.phabricator.com/D6682
Summary: Ref T3671. Depends on D6674. Continues work in D6673, D6674 and extends it into Legalpad and Phriction. Then deletes a bunch of dead code.
Test Plan: Edited documents in Legalpad and Phriction, verified I got reasonable looking previews.
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T3671
Differential Revision: https://secure.phabricator.com/D6675
Summary:
Ref T3671. A lot of applications have pretty ad-hoc preview code. Clean it up a bit and add Summary preview to Differential.
After ApplicationTransactions we might want to try to serialize the whole form and show a preview of all the transactions, but this seems not very useful in most cases (I'd guess that Remarkup previews are 99% of the value) and tricky to get right (e.g., adding images which don't exist yet to Pholio mocks).
I think I can add this in a few other places, too.
Test Plan:
Edited Maniphest Tasks and Differential Revisions, mashed some buttons. Verified previews rendered correctly. Grepped for removed CSS classes (no hits).
{F52907}
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T3671
Differential Revision: https://secure.phabricator.com/D6673
Summary:
Previously, if there were no macros, we would ping conduit for a list of macros until we got something. Now we cache false when there are no results.
T3045
Test Plan: Ensure the init doesn't call the ##macro.query## conduit method more than once during the PhabricatorBot's lifetime.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T3045
Differential Revision: https://secure.phabricator.com/D6671
Summary: Ref T3650. This adds a create transaction, transactions for metadata (title, langauge, view policy), and comments. Editor is used on all create /edit paths.
Test Plan: made some pastes via web and email - yay. edited pastes - yay. verified txns showed up on pastes and in feed correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3516, T3650
Differential Revision: https://secure.phabricator.com/D6645
Summary: Ref T3578. I forget if this was an explicit decision or not, but we currently let the same user answer questions multiple times. I think this probably causes more confusion than it provides freedom. In conjunction with other UI issues (commenting being weird, notably), we're seeing some use of answers to comment, which is undesirable. Require each answer's author to be unique. Merge existing nonunique authors' answers.
Test Plan: {F52062}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3578
Differential Revision: https://secure.phabricator.com/D6605
Summary:
Fixes T3579
As a basic overview, this enables the author of a question to open/close a question.
Other bits;
- Add "Open" filter to the builtin queries
- Add "Status" to search form
- Refactor ponder constants
- Add coloured bars for different question statuses
Test Plan:
- Open/Close questions
- Search for some bits
- Use filters
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3579
Differential Revision: https://secure.phabricator.com/D6590
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary: Like other icons, except white (hover states).
Test Plan: photoshop
Reviewers: epriestley, btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6591
Summary: Ref T2715. Had to start loading status information in the query class. Debated trying to clean up some of the attach / load stuff but decided to just add status under the new paradigm for now.
Test Plan: phid.query also made a status and checked that out. also played in conpherence.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6585
Summary: Ref T2715.
Test Plan: loaded conpherence, loaded a different thread, made a conpherence. also phid.query
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6577
Summary: ref T2715 - apologies btw as I didn't catch the "start from the bottom" ask until recently... :/
Test Plan: phid.query for some legalpad documents... booya. also loaded legalpad.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6576
Summary:
Now you can actually replace an image! Ref T3572. This ended up needing a wee bit of infrastructure to work...
- add replace image transaction to pholio
- add replacesImagePHID to PholioImage
- tweaks to editor to properly update images with respect to replacement
- add edges to track replacement
- expose getNodes on graph query infrastructure to query the entire graph of who replaced who
- move pholio image to new phid infrastructure
Still TODO - the history view should get chopped out a bit from the current view - no more inline comments / generally less functionality plus maybe a tweak or two to make this more sensical.
Test Plan: replaced images and played with history controller a little. works okay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3572
Differential Revision: https://secure.phabricator.com/D6560
Summary: Ref T2715. Switch Ponder to the new IDs.
Test Plan: Ran `phid.lookup`; `phid.query`. Grepped for old constant
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6554
Summary: Fixes T3610. This got un-scoped at some point, and is only used in Drydock so it escaped notice.
Test Plan: Ran `bin/drydock lease --type host` without hitting an exception about incorrect query construction.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3610
Differential Revision: https://secure.phabricator.com/D6552
Summary: Fixes T3557. One thing which made T3557 kind of a mess was the lack of information about progress through temporary failures. Add a column which records a task's last failure time, and surface it in the console.
Test Plan: {F51277}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3557
Differential Revision: https://secure.phabricator.com/D6550
Summary: Fixes T2569. This is the other common exception source which is ambiguous. List the task ID explicitly to make debugging easier.
Test Plan: {F51268}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2569
Differential Revision: https://secure.phabricator.com/D6549
Summary:
Ref T3557. This stuff does a bunch of nonsense in the View right now. Instead, do it in a real Query class.
Fixes a long-standing bug which prevented "all daemons" from showing more than 3 days' worth of data.
Test Plan: Viewed `/daemon/`, viewed "All Daemons".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3557
Differential Revision: https://secure.phabricator.com/D6544
Summary: Ref T1670. Use events and direct database writes instead of Conduit. Deprecate the Conduit methods.
Test Plan: Ran daemons, used the console to review daemon event logs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1670
Differential Revision: https://secure.phabricator.com/D6536
Summary: Ref T2715. `PhabricatorObjectQuery` can theoretically bypass policies on its side-channel result set. This can't actually happen in practice because all the loading mechanisms are filtered, but provide a general way to implement side channel results safely.
Test Plan: Loaded some pages; see next diff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6514
Summary: Ref T2715. Move Projects to the new stuff.
Test Plan: Used `phid.query` to load projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6526
Summary: Ref T2715. Move files to the new stuff.
Test Plan: Used `phid.query`; `phid.lookup` to find files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6523
Summary: Ref T2715. Switch mocks to the new stuff.
Test Plan: Used `phid.query` and `phid.lookup` to find mocks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6517
Summary: Ref T2715. Switch Maniphest to the new stuff.
Test Plan: Used `phid.query`; `phid.lookup` to load objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6516
Summary: Ref T2716. Ref T2715. Move CMIT to use Application PHIDs. Nothing too special here, but I consolidated some code into DiffusionCommitQuery. Depends on D6514.
Test Plan: Browsed Diffusion. Browsed Differential/Maniphest with linked commits. Used jump nav; used `phid.lookup` and `phid.query`. Used remarkup for Git and SVN repos. Grepped for PHID_TYPE_CMIT.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715, T2716
Differential Revision: https://secure.phabricator.com/D6515
Summary:
Ref T2715. Ref T3551. Ref T603. This does a few things, but they're all sort of small:
- We commonly use a `getX()` / `attachX()` pattern, but have very similar code in the `getX()` method every time. Provide a convenience method to make this pattern easier to write.
- We use `willFilterPage()` in many queries, but it currently is called with zero or more results. This means we have a lot of "if no results, return nothing" boilerplate. Make it call only for one or more results.
- Implement `PhabricatorPolicyInterface` on `ReleephBranch`. A branch has the same policy as its project.
- Implement `ReleephBranchQuery`.
- Move the branch PHID type to application PHID infrastructure.
Test Plan: Browsed Releeph. Used `phid.query` to query branch PHIDs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2715, T3551
Differential Revision: https://secure.phabricator.com/D6512
Summary:
See discussion in T2715. Currently, PHIDs are all hard coded in the PHID application. In the long run, we need to move them out into actual applications.
A specific immediate issue is Releeph, which uses a very very old and very broken mechanism to inject PHIDs in a way that only sort of works.
Moving forward, every PHID type will be provided by a `PhabricatorPHIDType` subclass, which will manage loading it, etc.
This also moves toward cleaning up the "load objects by name" (where "name" means something like `D12`) code, which is an //enormous// mess and spread across at least 4-5 callsites.
Test Plan: Used `phid.lookup` and `phid.query` to load Slowvotes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6502
Summary: Fixes T2654.
Test Plan: attached lots of mocks and tasks to one another from both maniphest and pholio. verified transactions rendered okay in both applications
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2654
Differential Revision: https://secure.phabricator.com/D6501
Summary:
When we delete a database, we still need to create it in the patch sequence so that installs can upgrade correctly. However, we shouldn't try to dump, probe, or list it. Mark deleted databases (of which there is only one) as "dead" and don't dump them.
A specific problem this fixes is `bin/storage dump` failing when trying to dump `phabricator_timeline`, which no longer exists.
Test Plan: Ran `bin/storage dump`, `list`, `probe`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6496
Summary:
Ref T1670. Mostly, use PhutilArgumentParser. This breaks up the mismash of functional stuff and PhabriatorDaemonControl into proper argumentparser Workflows.
There are no functional changes, except that I removed the "pingConduit()" call prior to starting daemons, because I intend to remove all Conduit integration.
Test Plan:
- Ran `phd list`.
- Ran `phd status` (running daemons).
- Ran `phd status` (no running daemons).
- Ran `phd stop <pid>` (dead task).
- Ran `phd stop <pid>` (live task).
- Ran `phd stop zebra` (invalid PID).
- Ran `phd stop 1` (bad PID).
- Ran `phd stop`.
- Ran `phd debug zebra` (no match).
- Ran `phd debug e` (ambiguous).
- Ran `phd debug task`.
- Ran `phd launch task`.
- Ran `phd launch 0 task` (invalid arg).
- Ran `phd launch 2 task`.
- Ran `phd help`.
- Ran `phd help list`.
- Ran `phd start`.
- Ran `phd restart`.
- Looked at Repositories (daemon running).
- Looked at Repositories (daemon not running).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1670
Differential Revision: https://secure.phabricator.com/D6490
Summary:
Not removing `phutil_render_tag()` for now as it is still used in Diviner.
@edward, please verify Facebook callsites.
Test Plan: Searched for it.
Reviewers: edward, epriestley
Reviewed By: epriestley
CC: aran, Korvin, wez
Differential Revision: https://secure.phabricator.com/D6494
Summary:
Fixes T2675, T2676.
- when the last person leaves a project it is archived.
- a script to archive all memberless projects
- better feed stories for the various policy edits you can do
- phriction pages are also moved as you rename projects
Test Plan: edited some projects and noted reasonable feed stories. ran script against test data and it worked! left a last man standing project and it archived. renamed a project to "a" then "b" then "a" (etc) and it worked including phrictiondocument moves
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2676, T2675
Differential Revision: https://secure.phabricator.com/D6478
Summary: Updates to the gradient logo and hashed background. Minor pixel tweaks.
Test Plan: Test desktop and mobile. Check photoshop for alignment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6487
Summary: Status icons for next to people names
Test Plan: photoshop
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2064
Differential Revision: https://secure.phabricator.com/D6479
Summary:
Nice title. We add three new transactions - IMAGE_FILE, IMAGE_NAME, and IMAGE_DESCRIPTION. The first is a bit like subscribers as it is a list of file phids. The latter have values of the form ($file_phid => $data), where $data is $name or $description respectively. This is because we need to collate transactions based on $file_phid...
Overall, this uses the _underyling files_ and not the "PholioImage" to determine if things are unique or not. That said, simply mark PholioImages as obsolete so inline comments about no-longer applicable PholioImages don't break.
Does a reasonable job implementing the mock. Note you can't "update" an image at this time, though you can delete and add at will.
Test Plan: played with pholio a ton.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3489
Differential Revision: https://secure.phabricator.com/D6441
Summary: Subscribing doesn't do anything yet, but you can subscribe!
Test Plan: {F50169}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6456
Summary:
Move comments from the old table to ApplicationTransactions. Patch dances around which objects it uses since I intend to delete the comment table.
NOTE: This temporarily disables comment writes. I'll restore them shortly.
Test Plan: {F50166}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6454
Summary: Schema changes to modernize this app.
Test Plan: Ran schema changes, created a new slowvote. No real effects yet.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6453
Summary: Fixes T3525. This feels way better, although it's still a little hard for me to pick out of lists with otherwise default-colored items.
Test Plan: {F49910} {F49911}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3525
Differential Revision: https://secure.phabricator.com/D6435
Summary: Fixes T3069. Ref T3516. For a long time, LDAP import added trailing whitespace to real names when importing them (for example, `"John Smith"` would be imported as `"John Smith "`). We no longer do this, but should clean up the database since it seems highly unlikely that any user wants trailing whitespace in their real name.
Test Plan:
$ ./bin/storage upgrade -f --apply phabricator:20130711.trimrealnames.php
Applying patch 'phabricator:20130711.trimrealnames.php'...
Trimming trailing whitespace from user real names...
Trimming user 1 from 'Evan Priestley ' to 'Evan Priestley'.
User 4 is already trim.
User 5 is already trim.
User 6 is already trim.
User 7 is already trim.
User 8 is already trim.
User 9 is already trim.
User 10 is already trim.
User 11 is already trim.
User 12 is already trim.
User 13 is already trim.
User 14 is already trim.
User 15 is already trim.
User 21 is already trim.
User 22 is already trim.
User 26 is already trim.
User 28 is already trim.
User 29 is already trim.
User 32 is already trim.
User 33 is already trim.
User 35 is already trim.
User 39 is already trim.
User 51 is already trim.
User 53 is already trim.
User 54 is already trim.
User 55 is already trim.
Done.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3069, T3516
Differential Revision: https://secure.phabricator.com/D6426
Summary:
Keep track of the state of a reviewer in an edge between reviewer and revision.
The edge stores the state of the review, added or rejected. And if the revision was
accepted by that reviewer the id of the diff accepted.
Test Plan:
Create diffs and clowncopterize reviewer list changes. This includes:
* Adding new reviewers
* Resigning
* Commandering a revision
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6372
Conflicts:
src/applications/differential/editor/DifferentialCommentEditor.php
Summary: Fixes T3481. Sort of - this thing be very ugly. Also it assumes that you'll "always" want to sign terms. I was thinking in a future diff that should be optional as well as configurable, though it was unclear to me if either was worth pursuing... Generally very hideous as the three main elements (PHUIDocument, AphrontErrorView, and AphrontForm with an AphrontFormInset) have never really played together before.
Test Plan: agreed to some test terms. noted UI displayed nicely. reloaded and noted UI told me I had signed it already. Went to different terms and filled them out wrong and got sensical errors.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3481
Differential Revision: https://secure.phabricator.com/D6399
Summary:
Ref T1703. Drive "user since" with a custom field and make the other fields render into a property list.
Users can make their profiles a little more personal/obnoxious now.
Also delete a bunch of code.
Test Plan: {F49415}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6401
Summary:
Depends on D6395.
- Now that inline rules have explicit priorities, they can just go in applications in all cases.
- We don't need the inline rule conditionals anymore after D6395.
Test Plan: Wrote remarkup with mentions, phriction links, countdowns, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6396
Summary: Ref T2852. When a Differential revision is linked to an Asana task, show the related task in Differential.
Test Plan: {F49234}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6387
Summary: Ref T1703. Put a more reasonable UI than "blob of JSON" on top of this.
Test Plan:
Reordered, enabled and disabled user profile fields.
{F49317}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6393
Summary: this let's the list controller save a query. Fixes T3488. Note didn't bother denormalizing document body at all since I don't think we want to show a snippet.
Test Plan: viewed a list of legalpad documents - yay. viewed a legalpad document - yay. created a legalpad document - yay. edited a legalpad document - yay. edited with N authors - yay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3488
Differential Revision: https://secure.phabricator.com/D6382
Summary:
Ref T2852. Asana sync tasks currently have a standard retry/backoff schedule, but the defaults are quite aggressive (retry every 60s forever). Instead, retry at increasing intervals and stop retrying after a few tries.
- Retry at intervals and stop retrying after a few iterations.
- Modernize some interfaces.
- Add better information about retry behaviors to the web UI.
Test Plan: {F49194}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6381
Summary:
Supports !unsubscribe and commenting on replies. Subscribers get mailed something reasonable. Fixes T3480.
Sneaks in /LX/ support. In the near future I want to have that /LX/ be a clean "signature" page sans all the edit actions and other fluff... Will resolve this as part of T3481.
Test Plan: used the metamta console to add comments and unsubscribe. added a phlog() inside mail code to verify mail bodies looked okay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3480
Differential Revision: https://secure.phabricator.com/D6369
