Summary:
Fixes T10912. When you drag tasks within a milestone, we currently apply an overbroad, API-focused rule and add the parent board's project. This logic was added fairly recently, as part of T6027, to improve the behavior of API-originated moves.
Later on, this causes the task to toggle in and out of the parent project on every alternate drag.
This logic is also partially duplicated in the `MoveController`.
- Add test coverage for this interaction.
- Fix the logic so it accounts for subproject / milestone columns correctly.
- Put all of the logic into the TransactionEditor, so the API gets the exact same rules.
Test Plan:
- Added a failing test and made it pass.
- Dragged tasks around within a milestone column:
- Before patch: they got bogus project swaps on every other move.
- After patch: projects didn't change (correct).
- Dragged tasks around between normal and milestone columns.
- Before patch: worked properly.
- After patch: still works properly.
Here's what the bad changes look like, the task is swapping projects with every other move:
{F1255957}
The "every other" is because the logic was trying to do this:
- Add both the parent and milestone project.
- Whichever one exists already gets dropped from the change list because it would have no effect.
- The other one then applies.
- In applying, it forces removal of the first one.
- Then this process repeats in the other direction the next time through.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10912
Differential Revision: https://secure.phabricator.com/D15834
Summary:
Ref T10748. This allows an EditEngine form to be broken up into pages.
This is less powerful than `PHUIPagedFormView`, because the pages are not sequential / stateful. Each form saves immediately once it's submitted, and can not take you to a new form or back/forward in a series of forms.
For example, you can't create a workflow where the user fills out 5 pages of information before we create an object, like the current repository workflow does.
However, the only place we've ever wanted to do this is repositories and it's fairly bad there, so I feel reasonably confident we aren't going to miss this in the future.
(We do "choose a type of service/repository/rule -> fill out one page of info" fairly often, but can do this without the full-power paging stuff.)
Test Plan:
- Created a repository usin the new Manage UI, filling out only a handful of fields.
- Edited a repository using the new Manage UI.
- All forms are now EditEngine forms offering paged views of the big huge underlying form:
{F1254371}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15832
Summary:
Ref T10748. Ref T10366. Allows users to set credential for new URIs.
- Ref T7221. Our handling of the "git://" protocol is currently incorrect. This protocol is not authenticated, but is considered an SSH protocol. In the new UI, it is considered an anonymous/unauthenticated protocol instead.
- Ref T10241. This fixes the `PassphraseCredentialControl` so it doesn't silently edit the value if the current value is not visible to you and/or not valid.
Test Plan:
Performed a whole lot of credential edits, removals, and adjustments. I'll give this additional vetting before cutting over to it.
{F1253207}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7221, T10241, T10366, T10748
Differential Revision: https://secure.phabricator.com/D15829
Summary: Fixes T10906, Fixes T10820. Adds new icons, grey-er colors for previous states. Also, I think fixed a few bugs?
Test Plan: Fake each state, verify icon is as intended.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10820, T10906
Differential Revision: https://secure.phabricator.com/D15830
Summary:
Ref T10748.
- Allow users to add new URIs by clicking a button instead of knowing a secret URI.
- Validate that URIs are actually valid URIs.
- Add enable/disable action and strings.
Test Plan:
- Created a new URI.
- Tried to create a nonsense URI, created a good URI.
- Enabled/disabled a URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15825
Summary: Ref T10748. Adds a "uris" attachment with URI information.
Test Plan: Queried URI information via Conduit, saw reasonable looking information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15822
Summary: Ref T10748. Brings the rest of the transactions to EditEngine, supports creating via API.
Test Plan:
- Created a URI via API.
- Created a URI via web.
- Tried to apply sneaky transactions, got rejected with good error messages. <_< >_>
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15821
Summary: Ref T10748. Ref T10366. This documents how everything is planned to work shortly.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler, scode
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15817
Summary:
Ref T10748.
- New View page for repository URIs.
- Make display and I/O behavior (observe, mirror, read, read/write) editable.
- Add a bunch of checks to prevent you from completely screwing up a repository by making it writable from a bunch of differnet sources.
Test Plan:
{F1249866}
{F1249867}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15816
Summary:
Ref T10748. Ref T10366. This adds a new EditEngine, EditController, Editor, Query, and Transaction for RepositoryURIs.
None of these really do anything helpful yet, and these URIs are still unused in the actual application.
Test Plan: {F1249794}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15815
Summary:
Ref T10748. Allow the new EditEngine workflow to create repositories by giving the user a modal repository type choice upfront.
(The rest of this flow is still confusing/weird, though.)
Test Plan:
- Created a new repository.
{F1249626}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15813
Summary: Ref T10748. This brings the "Actions" items (publish/notify + autoclose enabled) into the new UI.
Test Plan:
- Edited this stuff via EditEngine and Conduit.
- Viewed via new Manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15811
Summary: Ref T10748. Makes a "Branches" panel, enables these transactions in the EditEngine.
Test Plan:
- Edited via EditEngine + Conduit.
- Viewed via manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15809
Summary: Ref T10748. Port this, add EditEngine support, add some type validation to the transaction.
Test Plan:
- Edited via EditEngine.
- Edited via Conduit.
- Viewed via Management UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15808
Summary:
Ref T10843. There are actually two separate notions of cacheability here:
- Is this cacheable by the browser (e.g., should we emit "Expires: long in the future")?
- Is this cacheable locally (e.g., should we stick it in APC, or just read it off disk every time)?
These got a little mixed up by D15775, so we aren't currently emitting proper "Expires" headers on font files and a few other resource types.
Straighten this out so that we "Expires" these unusual resources correctly.
Test Plan: Verified that `.woff` files get a proper "Expires" header now, not just CSS/JS.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10843
Differential Revision: https://secure.phabricator.com/D15807
Summary: Ref T10748. Brings this forward in the UI and EditEngine.
Test Plan:
- Edited via Conduit.
- Viewed via Manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15805
Summary: Ref T10748. Ports this UI and exposes it on the EditEngine.
Test Plan:
- Edited via EditEngine.
- Viewed new manage UI.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15804
Summary:
Fixes T10778. This is a result of T10262: when we save a form configuration and adjust the policy, we try to scramble attached file secrets.
There aren't going to be any attached files, but there's also no edge table, so we fail.
We could skip this code, but we'll likely need an edge table here sooner or later so it's probably simpler in the long run to just add an empty one.
Test Plan:
- Ran `bin/storage upgrade`, got a clean bill of health.
- Saved a form configuration after making a policy edit, no more `edge` exception.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10778
Differential Revision: https://secure.phabricator.com/D15803
Summary: Ref T4292. This provides at least some sort of hint about how to set up cluster repositories.
Test Plan:
- Read documentation.
- Ran `bin/repository clusterize` to add + remove clusters.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15798
Summary:
Fixes T10876. Currently, we can end up with a setup warning banner sticking on each web device, since the state is stored in local cache.
Instead:
- When we actually run the setup checks, save the current state in the database.
- Before we show a cached banner, make sure the database still says the checks are a problem.
This could lead to some inconsistencies if setup checks legitimately pass on some hosts but not on others. For example, if you have `git` installed on one machine but not on another, we may raise a setup warning ("No Git Binary!") about it on one host only.
For now, assume users have their operational environments in some sort of reasonable shape and can install the same stuff everywhere. In the future, we could split the issues into "global" and "per-host" issues if we run into problems with this.
Test Plan:
This is somewhat tricky to test locally since you really need multiple webservers to test it properly, but I:
- Created some setup issues, saw banner.
- Ignored/cleared them, saw banner go away.
- Verified database cache writes were occurring properly.
Then I sort of faked it like this:
- Created a setup issue.
- Manually set the database cache value to `[]` ("no issues").
- Reloaded page.
- No more banner.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10876
Differential Revision: https://secure.phabricator.com/D15802
Summary: Ref T10748. Brings this over and adds EditEngine support for it.
Test Plan: Viewed and edited staging area information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15801
Summary: Ref T10748. This merges "Storage" and "Cluster" into a single UI which combines the information of both.
Test Plan: {F1246882}
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15800
Summary:
Ref T4292. This is a required step in configuring a cluster: document and explain it.
Previously `bin/almanac register` could //also// add and trust keys. I've removed this capability since I think it's needless and complicated. If there's some real use for it eventually, we could add a `bin/almanac add-key` or whatever. The workflow is simpler and has better guard rails that point you in the correct direction now.
Test Plan:
- Read documentation.
- Ran `bin/almanac` with various good/bad flags.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15795
Summary: This gets over-escaped instead of bolded right now, but I only ever hit it when exporting/importing and never both cleaning it up.
Test Plan: Ran `bin/repository move-paths`, saw bolded "Move" instead of ANSI escape sequences.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15797
Summary:
Ref T10860. This allows us to recover if the connection to the database is lost during a push.
If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it.
We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock.
Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again.
Basically, the changes are:
- If we can't release the lock at first, sit in a loop trying really hard to release it for a while.
- Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on.
- Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read.
Test Plan:
- Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur.
- Pushed like this:
```
$ echo D >> record && git commit -am D && git push
[master 707ecc3] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
BEGIN SLEEP
```
- Here, I stopped `mysqld` from the CLI in another terminal window.
```
END SLEEP
# CRITICAL. Failed to release cluster write lock!
# The connection to the master database was lost while receiving the write.
# This process will spend 300 more second(s) attempting to recover, then give up.
```
- Here, I started `mysqld` again.
```
# RECOVERED. Link to master database was restored.
# Released cluster write lock.
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
2cbf87c..707ecc3 master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15792
Summary:
Ref T10860. At least in Git over SSH, we can freely echo a bunch of stuff to stderr and Git will print it to the console, so we can tell users what's going on.
This should make debugging, etc., easier. We could tone this down a little bit once things are more stable if it's a little too chatty.
Test Plan:
```
$ echo D >> record && git commit -am D && git push
[master ca5efff] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 256 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
8616189..ca5efff master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15791
Summary: Ref T10860. This doesn't change anything, it just separates all this stuff out of `PhabricatorRepository` since I'm planning to add a bit more state to it and it's already pretty big and fairly separable.
Test Plan: Pulled, pushed, browsed Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15790
Summary:
Fixes T10865.
- Mock descriptions did not markup.
- Image descriptions did not get a proper container `<div />`.
Test Plan:
- Created a mock with remarkup in the mock description and in an image description.
- Viewed mock detail.
- Saw list styles render properly in both mock description and image description.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10865
Differential Revision: https://secure.phabricator.com/D15793
Summary: Fixes T10863. See that task for discussion.
Test Plan:
- Configured `aphlict` with no "logs".
- Started `aphlict`.
- Before change: exception.
- After change: worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10863
Differential Revision: https://secure.phabricator.com/D15788
Summary:
Ref T4292. Sometimes, we may not have a working copy for a repository. The easiest way to get into this condition is to deactivate a repository.
We could try to clone + fetch in this case, but that's kind of complex, and there's an easy command that administrators can run manually. For now, just tell them to do that.
This affects the inactive repositories on `secure`, like rGITCOINS.
Test Plan: Removed working copy, got message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15786
Summary:
Ref T7789. Currently, we use different viewers if you have `security.alternate-file-domain` configured vs if you do not.
This is largely residual from the days of one-time-tokens, and can cause messy configuration-dependent bugs like the one in T7789#172057.
Instead, always use the omnipotent viewer. Knowledge of the secret key alone is sufficient to access a file.
Test Plan:
- Disabled `security.alternate-file-domain`.
- Reproduced an issue similar to the one described on T7789.
- Applied change.
- Clean LFS interaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15784
Summary:
Ref T4292. When the daemons make a query for repository information, we need to make sure the working copy on disk is up to date before we serve the response, since we might not have the inforamtion we need to respond otherwise.
We do this automatically for almost all Diffusion methods, but this particular method is a little unusual and does not get this check for free. Add this check.
Test Plan:
- Made this code throw.
- Ran `bin/repository reparse --message ...`, saw the code get hit.
- Ran `bin/repository lookup-user ...`, saw this code get hit.
- Made this code not throw.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15783
Summary: I've possibly seen a couple of `aphlict` processes exit under suspicious circumstances (maybe?). Make sure any PHP errors get captured into the log.
Test Plan:
- Added an exception after forking.
- Before change: vanished into thin air.
- After change: visible in the log.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15782
Summary:
Fixes T10843. In a multi-server setup, we can do this:
- Two servers, A and B.
- You push an update.
- A gets pushed first.
- After A has been pushed, but before B has been pushed, a user loads a page from A.
- It generates resource URIs like `/stuff/new/package.css`.
- Those requests hit B.
- B doesn't have the new resources yet.
- It responds with old resources.
- Your CDN caches things. You now have a poisoned CDN: old data is saved in a new URL.
To try to avoid this with as little work as possible and generally make it hard to get wrong, check the URL hash against the hash we would generate.
If they don't match, serve our best guess at the resource, but don't cache it. This should make things mostly keep working during the push, but prevent caches from becoming poisoned, and everyone should get a working version of everything after the push finishes.
Test Plan:
- `curl`'d a resource, got a cacheable one.
- Changed the hash a little, `curl`'d again. This time: valid resource, but not cacheable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10843
Differential Revision: https://secure.phabricator.com/D15775
Summary: Ref T4292. This adds a new high-level overview panel.
Test Plan: {F1238854}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15772
Summary: Fixes T8897
Test Plan: Open any list view of Calendar events, every event should only show "Attending: ..." with users who are attending event.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8897
Differential Revision: https://secure.phabricator.com/D15779
Summary: This still wasn't quite right -- a link like `[[ Porcupine Facts ]]` with a space would not lookup correctly, and would render as `porcupine_facts`.
Test Plan: Verified that `[[ Porcupine Facts ]]` now works correctly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15780
Summary: Fixes T10845.
Test Plan: Verified that `[[ quack ]]` and `[[ QUACK ]]` both work. Previously, the link had to exactly match the capitalization of the target.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10845
Differential Revision: https://secure.phabricator.com/D15777
Summary: Fixes T10844. After recent changes to Aphlict (T6915 and T10697), `./bin/status` needs to be aware of the configuration file. As such, it is now necessary to run `./bin/aphlict status --config /path/to/config.json` rather than `./bin/aphlict status`.
Test Plan: Ran `./bin/aphlict start ...` and `./bin/aphlict status` and saw "Aphlict (`$PID`) is running".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10844
Differential Revision: https://secure.phabricator.com/D15776
Summary:
Fixes T8952. When Herald changes subscribers, it is zzzzz very boring.
When users change subscribers, it is still super boring (more boring than a merge, for example).
Test Plan: Viewed feed, saw fewer Herald stories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15774
Summary:
Ref T8952. Currently, when an application (most commonly Herald, but sometimes Drydock, Diffusion, etc) publishes a feed story, we get an empty grey box for it in feed.
Instead, give the story a little application icon kind of "profile picture"-like thing.
Test Plan:
Here's how it looks:
{F1239003}
Feel free to tweak/counter-diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15773
Summary:
Ref T10751. Add support tooling for manually prying your way out of trouble if disaster strikes.
Refine documentation, try to refer to devices as "devices" more consistently instead of sometimes calling them "nodes".
Test Plan: Promoted and demoted repository devices with `bin/repository thaw`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15768
Summary:
Ref T10751. Make the UI more useful and explain what failure states mean and how to get out of them.
The `bin/repository thaw` command does not exist yet, I'll write that soon.
Test Plan: {F1238241}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15766
Summary: Fixes T10744
Test Plan: Create recurring event, cancel one instance, cancel the parent event, reinstate event. Wording in the reinstating dialog should be clear about reinstating only instances that haven't been individually cancelled.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10744
Differential Revision: https://secure.phabricator.com/D15770
Summary: This is completely obsoleted by `owners.search`. See D15472.
Test Plan: Viewed API method in UI console.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15769
Summary:
Fixes T10840. When rendering mail, this rule wasn't falling through in quite the right way.
Also adjust where the rules are for this so the special styles show up in Maniphest, etc.
Test Plan:
Made this comment:
{F1238266}
Which produced this HTML:
{F1238267}
...and sent this mail:
{F1238283}
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T10840
Differential Revision: https://secure.phabricator.com/D15767
Summary: Ref T10751. This cleans this up so it's a little more modern, and fixes a possible bad access on the log detail page.
Test Plan: Viewed push log list, viewed push log detail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15765
Summary: This will stop breaking if you have subscribers and tags when updating a revision (`Error parsing field "Subscribers": The objects you have listed include objects which do not exist (Tags:)`), which I broke in D15749.
Test Plan: run through arc-diff --update that failed earlier.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15762
Summary:
Ref T4292. Right now, repository versions only get marked when a write happens.
This potentially creates a problem: if I pushed all the sync code to `secure` and enabled `secure002` as a repository host, the daemons would create empty copies of all the repositories on that host.
Usually, this would be fine. Most repositories have already received a write on `secure001`, so that working copy has a verison and is a leader.
However, when a write happened to a rarely-used repository (say, rKEYSTORE) that hadn't received any write recently, it might be sent to `secure002` randomly. Now, we'd try to figure out if `secure002` has the most up-to-date copy of the repository or not.
We wouldn't be able to, since we don't have any information about which node has the data on it, since we never got a write before. The old code could guess wrong and decide that `secure002` is a leader, then accept the write. Since this would bump the version on `secure002`, that would //make// it an authoritative leader, and `secure001` would synchronize from it passively (or on the next read or write), which would potentially destroy data.
Instead:
- Refuse to continue in situations like this.
- When a repository is on exactly one device, mark it as a leader with version "0".
- When a repository is created into a cluster service, mark its version as "0" on all devices (they're all leaders, since the repository is empty).
This should mean that we won't lose data no matter how much weird stuff we run into.
Test Plan:
- In single-node mode, used `repository update` to verify that `0` was written properly.
- With multiple nodes, used `repository update` to verify that we refuse to continue.
- Created a new repository, verified versions were initialized correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15761
Summary: Ref T4292. When we write a push log, also log which node received the request.
Test Plan: {F1230467}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15759
Summary:
Ref T4292. We currently synchronize hosted, clustered, Git repositories when we receive an SSH pull or push.
Additionally:
- Synchronize before HTTP reads and writes.
- Synchronize reads before Conduit requests.
We could relax Conduit eventually and allow Diffusion to say "it's OK to give me stale data".
We could also redirect some set of these actions to just go to the up-to-date host instead of connecting to a random host and synchronizing it. However, this potentially won't work as well at scale: if you have a larger number of servers, it sends all of the traffic to the leader immediately following a write. That can cause "thundering herd" issues, and isn't efficient if replicas are in different geographical regions and the write just went to the east coast but most clients are on the west coast. In large-scale cases, it's better to go to the local replica, wait for an update, then serve traffic from it -- particularly given that writes are relatively rare. But we can finesse this later once things are solid.
Test Plan:
- Pushed and pulled a Git repository over HTTP.
- Browsed a Git repository from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15758
Summary:
Ref T4292. Before we write or read a hosted, clustered Git repository over SSH, check if another version of the repository exists on another node that is more up-to-date.
If such a version does exist, fetch that version first. This allows reads and writes of any node to always act on the most up-to-date code.
Test Plan: Faked my way through this and got a fetch via `bin/repository update`; this is difficult to test locally and needs more work before we can put it in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15757
Summary:
Ref T4292. When you run `git fetch` and connect to, say, `repo001.west.company.com`, we'll look at the current version of the repository in other nodes in the cluster.
If `repo002.east.company.com` has a newer version of the repository, we'll fetch that version first, then respond to your request.
To do this, we need to run `git fetch repo002.east.company.com ...` and have that connect to the other host and be able to fetch data.
This change allows us to run `PHABRICATOR_AS_DEVICE=1 git fetch ...` to use device credentials to do this fetch. (Device credentials are already supported and used, they just always connect as a user right now, but these fetches should be doable without having a user. We will have a valid user when you run `git fetch` yourself, but we won't have one if the daemons notice that a repository is out of date and want to update it, so the update code should not depend on having a user.)
Test Plan:
```
$ PHABRICATOR_AS_DEVICE=1 ./bin/ssh-connect local.phacility.com
Warning: Permanently added 'local.phacility.com' (RSA) to the list of known hosts.
PTY allocation request failed on channel 0
phabricator-ssh-exec: Welcome to Phabricator.
You are logged in as device/daemon.phacility.net.
You haven't specified a command to run. This means you're requesting an interactive shell, but Phabricator does not provide an interactive shell over SSH.
Usually, you should run a command like `git clone` or `hg push` rather than connecting directly with SSH.
Supported commands are: conduit, git-lfs-authenticate, git-receive-pack, git-upload-pack, hg, svnserve.
Connection to local.phacility.com closed.
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15755
Summary:
Ref T4292. This consolidates code for figuring out which user we should connect to hosts with.
Also narrows a lock window.
Test Plan: Browsed Diffusion, pulled and pushed through an SSH proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15754
Summary:
In calendar, dashboard, diffusion, diviner, feed, fund,
maniphest, pholio, ponder, and slowvote use the term 'tags' if possible.
This intenctionally skips diffusion, differential, and the projects application itself.
Ref T10326 Ref T10349
Test Plan: inspection on a running, locally modified, system
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10835, T10326, T10349
Differential Revision: https://secure.phabricator.com/D15753
Summary:
Ref T10832. In practice, `git --version` is not a useful test for this issue:
- Vendors like Debian have backported the patch into custom versions like `0.0.0.1-debian-lots-of-patches.3232`.
- Vendors like Ubuntu distribute multiple different versions which report the same string from `git --version`, some of which are patched and some of which are not.
In other cases, we can perform an empirical test for the vulnerability. Here, we can not, because we can't write a 2GB path in a reasonable amount of time.
Since vendors (other than Apple) //generally// seem to be on top of this and any warning we try to raise based on `git --version` will frequently be incorrect, don't raise this warning.
I'll note this in the changelog instead.
Test Plan: Looked at setup issues, no more warning for vulnerable git version.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15756
Summary:
Ref T4292. Ref T10366. Depends on D15751. Today, generating repository commands is purely a function of the repository, so they use protocols and credentials based on the repository configuration.
For example, a repository with an SSH "remote URI" always generate SSH "remote commands".
This needs to change in the future:
- After T10366, repositories won't necessarily just have one type of remote URI. They can only have one at a time still, but the repository itself won't change based on which one is currently active.
- For T4292, I need to generate intracluster commands, regardless of repository configuration. These will have different protocols and credentials.
Prepare for these cases by separating out command construction, so they'll be able to generate commands in a more flexible way.
Test Plan:
- Added unit tests.
- Browsed diffusion.
- Ran `bin/phd debug pull` to pull a bunch of repos.
- Ran daemons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292, T10366
Differential Revision: https://secure.phabricator.com/D15752
Summary: Users can't find the "Tags" field in the Edit Menu; Added keyword "Tag".
Test Plan: Looked in Edit page; I think this shouldn't change anything else?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15749
Summary: Ref T4292. This will let the UI and future `bin/repository` tools give administrators more tools to understand problems when reporting or resolving them.
Test Plan:
- Pushed fully clean repository.
- Pushed previously-pushed repository.
- Forced write to abort, inspected useful information in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15748
Summary:
Ref T4292. Small fixes:
- There was a bug with the //first// write, where we'd write 1 but expect 0. Fix this.
- Narrow the window where we hold the `isWriting` lock: we don't need to wait for the client to finish.
- Release the lock even if something throws.
- Use a more useful variable name.
Test Plan:
- Made new writes to a fresh cluster repository.
- Made sequential writes.
- Made concurrent writes.
- Made good writes and bad writes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15747
Summary:
Fixes T10830.
- The return code from `storage adjust` did not propagate correct.
- There was one column issue which I missed the first time around because I had a bunch of unrelated stuff locally.
Test Plan:
- Ran `bin/storage upgrade -f` with failures, used `echo $?` to make sure it exited nonzero.
- Got fully clean `bin/storage adjust` by dropping all my extra local tables.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10830
Differential Revision: https://secure.phabricator.com/D15746
Summary: Ref T10832. Raise a setup warning for out-of-date versions of `git`.
Test Plan: {F1224632}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15745
Summary:
Fixes T10830. Ref T10366. I wasn't writing to this table yet so I didn't build it, but the fact that `bin/storage adjust` would complain slipped my mind.
- Add the table.
- Make the tests run `adjust`. This is a little slow (a few extra seconds) but we could eventually move some steps like this to run server-side only.
Test Plan: Ran `bin/storage upgrade -f`, got a clean `adjust`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10830
Differential Revision: https://secure.phabricator.com/D15744
Summary:
Ref T10748. This supports more transaction types in the modern editor and improves validation so Conduit benefits.
You can technically create repositories via `diffusion.repository.edit` now, although they aren't very useful.
Test Plan:
- Used `diffusion.repository.edit` to create and edit repositories.
- Used `/editpro/` to edit repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15740
Summary: Ref T10748. Ref T10366. No support for editing and no impact on the UI, but get some of the basics in place.
Test Plan: {F1223279}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15742
Summary: Ref T10748. Pretty straightforward. I'd like to put a little "!" icon in the menu if there's a warning/error eventually, but can deal with that latre.
Test Plan: {F1223096}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15741
Summary:
Ref T10748. This is roughly where I'm headed, if it makes some kind of sense? The "Edit" links in sub-sections don't work yet since I haven't built the thing.
Probably depends on D15736.
Test Plan: Manually navigated to `/manage/`, clicked around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15737
Summary: Ref T10748. Ref T10337. This technically implements this stuff, but it does not do anything useful yet. This skips all the hard stuff.
Test Plan:
- Technically used `diffusion.repository.search` to get repository information.
- Technically used `diffusion.repository.edit` to change a repository name.
- Used `editpro/` to edit a repository name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10337, T10748
Differential Revision: https://secure.phabricator.com/D15736
Summary: This error message is pointless and dead-ends logged-in users needlessly if they're sent to the register page by documentation or Advanced Enterprise Sales Funnels.
Test Plan: Visited `/auth/register/` while logged in, was sent home.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15739
Summary: Also make `../` work to start relative a link so I don't have to document it as `./../path`.
Test Plan:
- Used `./`, `../`. `./../`, and normal links (proper title pickup).
- Used bad links (red).
- Regenerated documentation:
{F1221692}
Reviewers: hach-que
Reviewed By: hach-que
Differential Revision: https://secure.phabricator.com/D15734
Summary: Ref T7691 (errata). This shows links to Phriction documents in red if they're missing, and links to Phriction documents in grey with a lock icon if the user doesn't have the correct permissions to see the document.
Test Plan:
Tested a bunch of different configurations:
```
[[ ./../ ]] Back to Main Document
[[ ./../subdocument_2]] Mmmm more documents
[[ ./../invisible_document]] Mmmm more documents
[[ ./../ | Explicit Title ]] Back to Main Document
[[ ./../subdocument_2 | Explicit Title ]] Mmmm more documents
[[ ./../invisible_document | Explicit Title ]] Mmmm more documents
[[ ]] Absolute link
[[ subdocument_2 ]] Absolute link
[[ invisible_document ]] Absolute link
[[ | Explicit Title ]] Absolute link
[[ subdocument_2 | Explicit Title ]] Absolute link
[[ invisible_document | Explicit Title ]] Absolute link
```
Got the expected result:
{F1221106}
Reviewers: epriestley, chad, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T7691
Differential Revision: https://secure.phabricator.com/D15733
Summary:
Resolves T7691. This turned out more complex than I really wanted, mainly because I needed to feed the slug information through to both the document renderer and the preview window that appears in the edit controller.
After this change, you can now create relative links in Phriction by doing `[[ ./../some/relative/path ]]`. Relative paths aren't handled anywhere else (they'll still render, but the dots are turned into a literal 'dot' as per existing behaviour).
Test Plan: Created some Phriction documents with relative links, saw them all link correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T7691
Differential Revision: https://secure.phabricator.com/D15732
Summary:
This nearly works but I didn't have time to get back to it and it isn't stable enough to turn on in the cluster yet.
We have enough other stuff going out this week, so just disable it before `stable` gets cut. Should be ready by next week if things go well.
Test Plan: Fetched a Git SSH repo locally.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15731
Summary: Ref T10697. I missed this so it isn't reading the new config properly.
Test Plan: Ran `bin/aphlict stop`, saw it read config.
Reviewers: chad, Mnkras
Reviewed By: Mnkras
Subscribers: Mnkras
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15729
Summary: Fixes T9348. If you have edit permission, you can kick people out of a room.
Test Plan:
- Kicked people out of a room.
- As an unprivileged user, wasn't able to kick people out of a room.
- Hit most (all?) of the various weird dialog sub-cases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9348
Differential Revision: https://secure.phabricator.com/D15728
Summary: Fixes T6806. We haven't seen users having issues with `phd.user` in a very long time.
Test Plan:
- Configured daemons to run as `notepriestley`, got a well-explained exception.
- Configured daemons to run as `epriestley`, got a clean start.
- Configured daemons with `phd.user=null`, got a clean start.
Reviewers: chad, areitz
Reviewed By: areitz
Subscribers: areitz
Maniphest Tasks: T6806
Differential Revision: https://secure.phabricator.com/D15726
Summary:
Ref T10756. This:
- Fixes T7307. This UI is now admin-only.
- Makes the main "running daemons" table more useful for multi-host setups (show where daemons are running).
- Removes logs from the web UI: these are sometimes vaguely sensitive and shouldn't be visible. The UI tells you how to get them with `bin/phd log`.
- Minor modernization.
Test Plan:
- As a non-admin, viewed daemons (access error) and bulk jobs (worked great).
- Browsed bulk job pages.
- Ran a bulk job.
- Viewed daemon console.
- Viewed task detail / daemon detail / daemon list pages.
{F1220516}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7307, T10756
Differential Revision: https://secure.phabricator.com/D15724
Summary: Fixes T10706
Test Plan: Open UI Example of badges. Shouldn't be broken
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: Korvin
Maniphest Tasks: T10706
Differential Revision: https://secure.phabricator.com/D15723
Summary:
Fixes T9385. This was accidentally mangled a bit a long time ago by D12797, which was a 1,000-file change which got almost everything right.
Simplify the message and fix all the `%s` conversions and how they map to parameters.
Test Plan: {F1220400}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9385
Differential Revision: https://secure.phabricator.com/D15722
Summary:
Ref T10696. By default, `node` uses 1.5GB, which is enormous overkill for this service and can crowd out other services if it's running next to things like a database on the same host.
Provide a configuration option to adjust it via `--max-old-space-size` and default to 256MB. It only seems to need about 30M locally, so this should be plenty of headroom.
Test Plan:
Ran `bin/aphlict debug`, things seemed OK.
It takes a long time (days?) to grow to 1.5GB so I can't easily test this locally without a lot of work, but I'll keep an eye on it in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10696
Differential Revision: https://secure.phabricator.com/D15720
Summary: Fixes T10816. The way these work is a little unusual since these chunks of file-rendering code are unusuall performance-sensitive, so the Differential version doesn't adapt directly to Diffusion. Both can possibly be unified at some point in the future, although they do slightly different things.
Test Plan: {F1220170}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10816
Differential Revision: https://secure.phabricator.com/D15719
Summary:
Ref T6915. This allows multiple notification servers to talk to each other:
- Every server has a list of every other server, including itself.
- Every server generates a unique fingerprint at startup, like "XjeHuPKPBKHUmXkB".
- Every time a server gets a message, it marks it with its personal fingerprint, then sends it to every other server.
- Servers do not retransmit messages that they've already seen (already marked with their fingerprint).
- Servers learn other servers' fingerprints after they send them a message, and stop sending them messages they've already seen.
This is pretty crude, and the first message to a cluster will transmit N^2 times, but N is going to be like 3 or 4 in even the most extreme cases for a very long time.
The fingerprinting stops cycles, and stops servers from sending themselves copies of messages.
We don't need to do anything more sophisticated than this because it's fine if some notifications get lost when a server dies. Clients will reconnect after a short period of time and life will continue.
Test Plan:
- Wrote two server configs.
- Started two servers.
- Told Phabricator about all four services.
- Loaded Chrome and Safari.
- Saw them connect to different servers.
- Sent messages in one, got notifications in the other (magic!).
- Saw the fingerprinting stuff work on the console, no infinite retransmission of messages, etc.
(This pretty much just worked when I ran it the first time so I probably missed something?)
{F1218835}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6915
Differential Revision: https://secure.phabricator.com/D15711
Summary: Fixes T10812. Make it easier to disambiguate great passwords like `iI|l1oO()thenumber1nospellitout`.
Test Plan: {F1219074}
Reviewers: chad, yelirekim
Reviewed By: yelirekim
Maniphest Tasks: T10812
Differential Revision: https://secure.phabricator.com/D15715
Summary: Updating the subproject and member pages in Projects to new UI
Test Plan: Visit a subproject parent page, visit members pages
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15687
Summary: Default to "All" (maybe "Active" in the future). Adds more info to results.
Test Plan: visit /phurl/, see additional information about URL
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15713
Summary: Fixes T10806. Although browsers don't seem to care about this, it's more correct to support it, and the new test console uses normal `cURL` and does care.
Test Plan:
- Hit the error case for providing a chain but no key/cert.
- Used `openssl s_client -connect localhost:22280` to connect to local Aphlict servers.
- With SSL but no chain, saw `openssl` fail to verify the remote.
- With SSL and a chain, saw `openssl` verify the identify of the remote.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10806
Differential Revision: https://secure.phabricator.com/D15709
Summary:
Ref T10809. Currently, both the proxy and target may mutate URIs (rewriting "svn+ssh://x/diffusion/Y/" to a path on disk).
I believe this previously worked by fate/chance/luck since both URI variants contain the repository information, but the algorithms were tightened up recently with callsign removal.
Stop rewriting them if we're the intracluster proxy -- they only need to be rewritten on the target host.
Test Plan:
- Checked out a proxied SVN repository, with and without a callsign.
- Checked out an unproxied SVN repository, with and without a callsign.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10809
Differential Revision: https://secure.phabricator.com/D15712
Summary:
Fixes T10783 (what little of it remains). Ref T10697.
Aphlict currently uses request paths for two different things:
- multi-tenant instancing in the Phacility cluster (each instance gets its own namespace within an Aphlict server);
- some users configure nginx and apache to do proxying or SSL termination based on the path.
Currently, these can collide.
Put a "~" before the instance name to make it unambiguous. At some point we can possibly just use a GET parameter, but I think there was some reason I didn't do that originally and this sequence of changes is disruptive enough already.
Test Plan: Saw local Aphlict unambiguously recognize "local.phacility.com" as instance "local", with a "~"-style URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697, T10783
Differential Revision: https://secure.phabricator.com/D15705
Summary:
Fixes T10697. This finishes bringing the rest of the config up to cluster power levels.
Phabricator is now given an arbitrarily long list of notification servers.
Each Aphlict server is given an arbitrarily long list of ports to run services on.
Users are free to make them meet in the middle by proxying whatever they want to whatever else they want.
This should also accommodate clustering fairly easily in the future.
Also rewrote the status UI and changed a million other things. 🐗
Test Plan:
{F1217864}
{F1217865}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15703
Summary: Ref T10697. Mostly straightforward. Also allow the server to have multiple logs and log options in the future (e.g., different verbosities or separate admin/client logs or whatever). No specific plans for this, but the default log is pretty noisy today.
Test Plan: Set up a couple of logs, started server, saw it log to them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15702
Summary: Ref T10697. This isn't everything but starts generalizing options and moving us toward a cluster-ready state of affairs.
Test Plan: Started server in various configurations, hit most (all?) of the error cases with bad configs, sent test notifications.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15701
Summary: Ref T10697. This just improves a couple of minor `bin/aphlict` things: make argument parsing more explicit/consistent, consolidate a little bit of duplicated code.
Test Plan: Ran all `bin/aphlict` commands.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15698
Summary:
Ref T2783. This allows this worker to run on a machine different to the one that stores the repository, by routing the execution of Git over Conduit calls.
This API method is super gross, but fixing it isn't straightforward and it runs into other complicated considerations. We can fix it later; for now, just define it as "internal" to limit how much mess this creates.
"Internal" methods do not appear on the console.
Test Plan: Ran `bin/repository reparse --change <commit> --trace` on several commits, saw daemons make a Conduit call instead of running a `git` command.
Reviewers: hach-que, chad
Reviewed By: chad
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11874
Summary: Fixes T10797. This seems to fix things on my local system.
Test Plan:
- Cloned with a username, got prompted for a password.
- Cloned with a username + password.
- Cloned with a username + bad password (error).
Reviewers: chad
Reviewed By: chad
Subscribers: Grimeh
Maniphest Tasks: T10797
Differential Revision: https://secure.phabricator.com/D15706
Summary:
Ref T10784. Currently, if you terminate SSL at a load balancer (very common) and use HTTP beyond that, you have to fiddle with this setting in your premable or a `SiteConfig`.
On the balance I think this makes stuff much harder to configure without any real security benefit, so don't apply this option to intracluster requests.
Also document a lot of stuff.
Test Plan: Poked around locally but this is hard to test outside of a production cluster, I'll vet it more thoroughly on `secure`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10784
Differential Revision: https://secure.phabricator.com/D15696
Summary:
Ref T10784. On `secure`, logged-out users currently can't browse repositories when cluster/service mode is enabled because they aren't permitted to make intracluster requests.
We don't allow totally public external requests (they're hard to rate limit and users might write bots that polled `feed.query` or whatever which we'd have no way to easily disable) but it's fine to allow intracluster public requests.
Test Plan: Browsed a clustered repository while logged out locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10784
Differential Revision: https://secure.phabricator.com/D15695
Summary: Fixes T10772, not sure why this fails, but reverting the code back to old dialog call works.
Test Plan:
- Try to add a new credential when importing a repository.
- Also created a new credential normally, via Passphrase.
- Also edited a credential.
Reviewers: chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T10772
Differential Revision: https://secure.phabricator.com/D15691
Summary:
Ref T10751. We currently have a placeholder Almanac document, and a fairly-bad-advice section in Daemons.
Pull these into the modern cluster documentation.
Test Plan: 17 phabricator PHDs
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15689
Summary: Fixes T10789. If we aren't configured with a device, we never grabbed a lock in the first place, and should not expect one to be held.
Test Plan: Pushed non-cluster-configured Git SSH repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10789
Differential Revision: https://secure.phabricator.com/D15692
Summary:
Ref T4292. This mostly implements the locking/versioning logic for multi-master repositories. It is only active on Git SSH pathways, and doesn't actually do anything useful yet: it just does bookkeeping so far.
When we read (e.g., `git fetch`) the logic goes like this:
- Get the read lock (unique to device + repository).
- Read all the versions of the repository on every other device.
- If any node has a newer version:
- Fetch the newer version.
- Increment our version to be the same as the version we fetched.
- Release the read lock.
- Actually do the fetch.
This makes sure that any time you do a read, you always read the most recently acknowledged write. You may have to wait for an internal fetch to happen (this isn't actually implemented yet) but the operation will always work like you expect it to.
When we write (e.g., `git push`) the logic goes like this:
- Get the write lock (unique to the repository).
- Do all the read steps so we're up to date.
- Mark a write pending.
- Do the actual write.
- Bump our version and mark our write finished.
- Release the write lock.
This allows you to write to any replica. Again, you might have to wait for a fetch first, but everything will work like you expect.
There's one notable failure mode here: if the network connection between the repository node and the database fails during the write, the write lock might be released even though a write is ongoing.
The "isWriting" column protects against that, by staying locked if we lose our connection to the database. This will currently "freeze" the repository (prevent any new writes) until an administrator can sort things out, since it'd dangerous to continue doing writes (we may lose data).
(Since we won't actually acknowledge the write, I think, we could probably smooth this out a bit and make it self-healing //most// of the time: basically, have the broken node rewind itself by updating from another good node. But that's a little more complex.)
Test Plan:
- Pushed changes to a cluster-mode repository.
- Viewed web interface, saw "writing" flag and version changes.
- Pulled changes.
- Faked various failures, got sensible states.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15688
Summary:
Ref T4292. This adds some very basic cluster/device data to the new management view. Nothing interesting yet.
Also deal with disabled bindings a little more cleanly.
Test Plan: {F1214619}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15685
Summary:
Ref T4292. This puts a very rough skeleton in place for the new "Manage Repository" UI, somewhat similar to the "Settings" UI.
Right now, it has one panel with no content, and is not reachable from the UI.
Test Plan: {F1214525}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15683
Summary:
Ref T10756. When repositories are properly configured for the cluster (which is hard to set up today), be smart about which repositories are expected to exist on the current host, and only pull them.
This generally allows daemons to pretty much do the right thing no matter how many copies are running, although there may still be some lock contention issues that need to be sorted out.
Test Plan: {F1214483}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10756
Differential Revision: https://secure.phabricator.com/D15682
Summary: Ref T10702
Test Plan: Open a user profile, attempt to award an archived or previously awarded badge, badges dialog should provide a typeahead, and the suggestions should offer details about whether a badge is archived or already awarded.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10702
Differential Revision: https://secure.phabricator.com/D15665
Summary:
Ref T4571. Write more of the missing documentation sections and clarify a few things.
Since the "replicating master" check needs a special permission, imposes a performance penalty, is probably very difficult to misconfigure, and likely not a big deal anyway, just drop the idea of trying to automatically detect + prevent it. We still show if it's an issue on the status page, provided we have permission to check.
When you don't have any cluster databases configured, never stop trying to connect to the default master database. We might want to do this eventually as load reduction, but just don't muddy the waters too much for now while things stabilize.
Test Plan:
- Tested functionality in cluster, non-cluster, and degraded-cluster modes.
- Used status console to monitor a health check cycle.
- Read docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15679
Summary:
Ref T4571. When a database goes down briefly, we fall back to replicas.
However, this fallback is slow (not good for users) and keeps sending a lot of traffic to the master (might be bad if the root cause is load-related).
Keep track of recent connections and fully degrade into "severed" mode if we see a sequence of failures over a reasonable period of time. In this mode, we send much less traffic to the master (faster for users; less load for the database).
We do send a little bit of traffic still, and if the master recovers we'll recover back into normal mode seeing several connections in a row succeed.
This is similar to what most load balancers do when pulling web servers in and out of pools.
For now, the specific numbers are:
- We do at most one health check every 3 seconds.
- If 5 checks in a row fail or succeed, we sever or un-sever the database (so it takes about 15 seconds to switch modes).
- If the database is currently marked unhealthy, we reduce timeouts and retries when connecting to it.
Test Plan:
- Configured a bad `master`.
- Browsed around for a bit, initially saw "unrechable master" errors.
- After about 15 seconds, saw "major interruption" errors instead.
- Fixed the config for `master`.
- Browsed around for a while longer.
- After about 15 seconds, things recovered.
- Used "Cluster Databases" console to keep an eye on health checks: it now shows how many recent health checks were good:
{F1213397}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15677
Summary:
Ref T4571. If we fail to connect to the master, automatically try to degrade into a temporary read-only mode ("UNREACHABLE") for the remainder of the request, if possible.
If the request was something like "load the homepage", that'll work fine. If it was something like "submit a comment", there's nothing we can do and we just have to fail.
Detecting this condition imposes a performance penalty: every request checks the connection and gives the database a long time to respond, since we don't want to drop writes unless we have to. So the degraded mode works, but it's really slow, and may perpetuate the problem if the root issue is load-related.
This lays the groundwork for improving this case by degrading futher into a "SEVERED" mode which will persist across requests. In the future, if several requests in a short period of time fail, we'll sever the database host and refuse to try to connect to it for a little while, connecting directly to replicas instead (basically, we're "health checking" the master, like a load balancer would health check a web application server). This will give us a better (much faster) degraded mode in a major service disruption, and reduce load on the master if the root cause is load-related, giving it a better chance of recovering on its own.
Test Plan:
- Disabled master in config by changing the host/username, got degraded automatically to UNREACAHBLE mode immediately.
- Faked full SEVERED mode, requests hit replicas and put me in the mode properly.
- Made stuff work, hit some good pages.
- Hit some non-cluster pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15674
Summary: Ref T4571. If `cluster.databases` is configured but only has replicas, implicitly drop to read-only mode and send writes to a replica.
Test Plan:
- Disabled the `master`, saw Phabricator automatically degrade into read-only mode against replicas.
- (Also tested: explicit read-only mode, non-cluster mode, properly configured cluster mode).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15672
Summary:
Ref T4571. Allows users to click the "read-only mode" notification to get more information about why an install is in read-only mode.
Installs can be in this mode for several reasons (explicit administrative action, no masters defined, no masters reachable), and it's useful to be able to tell the difference.
Test Plan: {F1212930}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15671
Summary:
Ref T4571. Ref T10759. Ref T10758. This isn't complete, but gets most of the job done:
- When `cluster.databases` is set up, most things ignore `mysql.host` now.
- You can `bin/storage upgrade` and stuff works.
- You can browse around in the web UI and stuff works.
There's still a lot of weird tricky stuff to navigate, and this has real no advantages over configuring a single server yet (no automatic failover, etc).
Test Plan:
- Configured `cluster.databases` to point at my `t1.micro` hosts in EC2 (master + replica).
- Ran `bin/storage upgrade`, got a new install setup on them properly.
- Survived setup warnings, browsed around.
- Switched back to local config, ran `bin/storage upgrade`, browsed around, went through setup checks.
- Intentionally broke config (bad hosts, no masters) and things seemed to react reasonably well.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571, T10758, T10759
Differential Revision: https://secure.phabricator.com/D15668
Summary: Ref T4571. The configuration option still doesn't do anything, but add a status panel for basic setup monitoring.
Test Plan:
Here's what a good version looks like:
{F1212291}
Also faked most of the errors it can detect and got helpful diagnostic messages like this:
{F1212292}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15667
Summary:
Ref T4571. This adds a new option which allows you to upgrade your one-host configuration to a multi-host configuration by configuring it.
Doing this currently does nothing. I wrote a lot of words about what it is //supposed// to do in the future, though.
Test Plan:
- Tried to configure the option in all the possible bad ways, got errors.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15663
Summary:
Ref T4571. There will be a very long path beyond this, but add a basic read-only mode. You can explicitly enable this to put Phabricator in a sort of "maintenance" mode today if you're swapping databases or something.
In the long term, we'll automatically degrade into this mode if the master database is down.
Test Plan:
- Enabled read-only mode.
- Browsed around.
- Didn't immediately see anything that was totally 100% broken.
Most stuff is 80-90% broken right now. For example:
- Stuff like submitting comments doesn't work, and gives you a confusing, unhelpful error.
- None of the UI really knows that it's read-only. EditEngine stuff should all hide itself and say "you can't add new comments while an install is in read-only mode", for example, but currently does not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15662
Summary: Testing out a new 'nav' layout in Settings / Config. Spent a few days here and couldn't find much better overall.
Test Plan: View each page in Settings and in Config. Save some config options. Test mobile, desktop, tablet.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15659
Summary: Fixes T5813, while I'm in here...
Test Plan: Sorted stuff by end date.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5813
Differential Revision: https://secure.phabricator.com/D15657
Summary: Fixes T10684. Fixes T10520. This primarily implements a date/epoch field, and then does a bunch of standard plumbing.
Test Plan:
- Created countdowns.
- Edited countdowns.
- Used HTTP prefilling.
- Created a countdown ending on "Christmas Morning", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10520, T10684
Differential Revision: https://secure.phabricator.com/D15655
Summary: Closes T10690
Test Plan: Open Badges application, go to Advanced Search, search for a badge by its name and see result.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T10690
Differential Revision: https://secure.phabricator.com/D15656
Summary:
Fixes T8613. This was pretty straightforward, I just never dug into it originally.
`rawResultLimit = 0` just means "no limit", so the fix is to only apply a limit if it is set to some nonzero value.
Also modernize a few pieces of code.
Test Plan: I'm actually not sure this can actually be hit normally? I faked `setGenerateGhosts(true)` into an unrelated query, hit the fatal, then fixed it.
Reviewers: lpriestley, chad
Reviewed By: chad
Maniphest Tasks: T8613
Differential Revision: https://secure.phabricator.com/D15653
Summary: Found another bouncing around.
Test Plan: Review in diff
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15651
Summary: Ref T10262. Instead of dumping an unhelpful 403 "ACCESS DENIED" page on users, explain the most likely cause of the issue and give them a link to return to the file detail page to learn more or get an up-to-date link.
Test Plan: Hit both errors, had a lovely experience with the helpful dialog text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15650
Summary: Ref T6027. We got a not-very-user-friendly default string before.
Test Plan: Selected "Move", didn't change the dropdown, hit submit. Now, got a nice human-readable description of the issue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15649
Summary: Bumps to 14px, fixes some on Differential
Test Plan: view various headers in Differential
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15647
Summary:
Ref T10262. Currently, we always render a tag like this when you `{F123}` an image in remarkup:
```
<img src="/xform/preview/abcdef/" />
```
This either generates the preview or redirects to an existing preview. This is a good behavior in general, because the preview may take a while to generate and we don't want to wait for it to generate on the server side.
However, this flickers a lot in Safari. We might be able to cache this, but we really shouldn't, since the preview URI isn't a legitimately stable/permanent one.
Instead, do a (cheap) server-side check to see if the preview already exists. If it does, return a direct URI. This gives us a stable thumbnail in Safari.
Test Plan:
- Dragged a dog picture into comment box.
- Typed text.
- Thing didn't flicker like crazy all the time in Safari.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15646
Summary: Going to render these all normal case instead of all caps, and bump up the font size. Should be more consistent. Yellow if you green anything orange.
Test Plan: grep, lint
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15645
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
Summary:
Ref T10262. Files have an internal secret key which is partially used to control access to them, and determines part of the URL you need to access them. Scramble (regenerate) the secret when:
- the view policy for the file itself changes (and the new policy is not "public" or "all users"); or
- the view policy or space for an object the file is attached to changes (and the file policy is not "public" or "all users").
This basically means that when you change the visibility of a task, any old URLs for attached files stop working and new ones are implicitly generated.
Test Plan:
- Attached a file to a task, used `SELECT * FROM file WHERE id = ...` to inspect the secret.
- Set view policy to public, same secret.
- Set view policy to me, new secret.
- Changed task view policy, new secret.
- Changed task space, new secret.
- Changed task title, same old secret.
- Added and ran unit tests which cover this behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15641
Summary:
This reverts commit 3f50ba90f1.
Fixes T10412. Everyone seems to hate this and I don't feel strongly about it. It's definitely a little weird.
Test Plan: Straight revert.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10412
Differential Revision: https://secure.phabricator.com/D15644
Summary: Fixes T10737. I agree that this is a better behavior than always making them blue (boring).
Test Plan:
what
{F1208333}
wut
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T10737
Differential Revision: https://secure.phabricator.com/D15643
Summary:
Ref T6027. Fixes T10734.
- If one of the projects a task is tagged with isn't visible to the user or doesn't have a board, it won't have columns.
- Don't show options for projects with disabled boards.
Test Plan:
- Viewed task with project with no columns; no fatal.
- Viewed task with project with disabled board; no options to move on that board.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10734, T6027
Differential Revision: https://secure.phabricator.com/D15640
Summary:
Ref T6027. Normally, actions use the same order as the form, but in some cases (like moving stuff on workboards) it makes sense to reorder them explicitly.
Pin "Move on board" near the bottom, and "projects/subscribers" at the bottom. I think these are generally reasonable rules in all cases.
Test Plan: Opened menu, saw slightly better action order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15639
Summary:
Ref T6027. Try this out and see how it feels? Clear issues:
- This definitely shouldn't be at the top.
- You should probably be able to select it multiple times?
- Some of the "which columns show up" rules might need adjustment?
- Diamond marker maybe not great?
Not sure I love this but it doesn't feel //terrible//...
Test Plan: {F1207891}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15638
Summary: Ref T5214. Fixes T10486. Ref T6027. This exposes the `TYPE_COLUMNS` transaction in a usable way via API, and fixes the interactions via prefilling.
Test Plan:
- Created tasks directly into columns via API.
- Moved tasks between columns via API.
- Used `?column=...` to try to create a template task with valid and bogus column PHIDs.
Reviewers: chad
Reviewed By: chad
Subscribers: AmyLewis
Maniphest Tasks: T5214, T6027, T10486
Differential Revision: https://secure.phabricator.com/D15636
Summary: Ref T6027. This adds human-readable rendering for the new `TYPE_COLUMNS` core transactions.
Test Plan: {F1207784}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15635
Summary:
Ref T6027. We currently have two different transaction types:
- `TYPE_PROJECT_COLUMNS` does most of the work, but has a sort of weird structure and isn't really suitable for API use.
- `TYPE_COLUMN` is this weird, junk transaction which mostly just creates the other transaction.
Merge them into a single higher-level `TYPE_COLUMNS` transaction which works properly and has a sensible structure and comprehensive error checking.
Remaining work here:
- I've removed the old rendering logic, but not yet added new logic. I need to migrate the old transaction types and add new rendering logic.
- Although the internal representation is now //suitable// for use in the API, it isn't properly exposed yet.
Test Plan:
- Created tasks into a column.
- Ran unit tests.
- Moved tasks between columns.
- Will perform additional testing in followups.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15634
Summary: Ref T10671
Test Plan: Open Conduit application, open `badges.edit` or `badges.search`, create, edit, or query for a badge.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10671
Differential Revision: https://secure.phabricator.com/D15622
Summary: Modernize Files a bit, use newPage
Test Plan: New file, drag and drop file, view file, edit file
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15631
Summary: Ref T7673. This is really just so I can force admin.phacility.com logout when you log out of an instance, but there are a few other things we could move here eventually, like the WILLREGISTERUSER event.
Test Plan: Logged out of an instance, got logged out of parent (see next change).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7673
Differential Revision: https://secure.phabricator.com/D15629
Summary:
Ref T7303. Currently, our handling of "scope" is fairly rigid and adheres to the spec, but some of these behaviors don't make much sense in practice.
Soften some behaviors and make them more flexible:
**Soft Failure on Unknown Permissions**: If a client asks for a permission we don't know about, just warn that we don't recognize it instead of fataling. In particular, I plan to make `offline_access` and `whoami` implicit. Older clients that request these permissions will still work fine as long as we don't hard-fatal.
**Move `user.whoami` to ALWAYS scope**: Make `whoami` a default permission. We've already done this, in effect; this just formalizes it.
**Tokens no longer expire**: Make `offline_access` (infinite-duration tokens) a default permission. I think the OAuth model doesn't map well to reality. It is common for other providers to issue "temporary" tokens with a duration of multiple years, and the refesh workflow is sort of silly. We can add a "temporary" scope later if we need temporary tokens.
This flow was potentially extra silly with the "log out of Phacility" use case, where we might need to have you log in again before we could log you out, which is bizarre and senseless. Avoid this nonsense.
**Move away from granular permissions**: Users currently get to pick-and-choose which permissions they grant, but this likely rarely/never works in practice and is fairly hostile since applications can't communicate which permissions they need. Applications which can actually operate with only some subset of permissions can make separate requests (e.g., when you activate "cool feature X", it asks for X permission). I think applications that do this are rare; pretty much everything just asks for tons of permissions and everyone grants them.
Making this all-or-nothing is better for well-behaved applications and better for users. It's also slightly better for overzealous applications that ask for more than they need, but whatever. Users can make an informed decision, hopefully, and I plan to let administrators force applications to a subset of permissions once we introduce meaningful scopes.
Test Plan:
- Generated tokens.
- Used tokens.
- Authorized an instance.
- Faked some bogus scopes, got clean authorization.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15621
Summary: Converts over to `newPage`
Test Plan: Pull up Settings panel, test a few.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15625
Summary: Ref T7303. This interaction is very oldschool; modernize it to enable/disable instead of "nuke from orbit".
Test Plan:
- Enabled applications.
- Disabled applications.
- Viewed applications in list view.
- Generated new tokens.
- Tried to use a token from a disabled application (got rebuffed).
- Tried to use a token from an enabled application (worked fine).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15620
Summary: Runs through Releeph to move to new UI and `newPage`
Test Plan: Ran through product, release, branch, everything seems to work.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15623
Summary: Swaps over to new hotness
Test Plan: Pull up mail view, see new UI
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15616
Summary: Swaps over to `newPage` and `newDialog`
Test Plan: Unsure how to actually pull these up?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15617
Summary: Updates to use new UI
Test Plan: Save a custom query, edit a custom query
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15618
Summary: Updates Console and Operations page.
Test Plan: Pull up Console, pull up status page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15615
Summary: Moves these Maniphest pages over to modern UI, components
Test Plan: Batch Edit Tasks, View some reports.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15614
Summary: Ref T7303. This application is currently stone-age tech (no transactions, hard "delete" action). Bring it up to modern specs.
Test Plan:
- Created and edited an OAuth application.
- Viewed transaction record.
- Tried to create something with no name, invalid redirect URI, etc. Was gently rebuffed with detailed explanatory errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15609
Summary: View various conduit pages and update to new UI and add calls to newPage
Test Plan: View list, view method, make a call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15613
Summary: Uses modern UI, `newPage`, etc. Changes table behavior to always scroll if too large for container, can't find anything this breaks, but be on the lookout.
Test Plan: Pull up help and view pages, search for some people and projects.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15611
Summary: Single callsite, swap to `newPage`
Test Plan: Visit page, see same status message. Also remove device ready flag.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15610
Summary:
Fixes T10721. When trying to load commits by identifier, we would take some bad pathways in Subversion if the repository had no callsign and end up missing the commits.
Fix this logic so it works for either callsigns (e.g., if passed `rXyyy`) or with PHIDs if passed repositories.
Test Plan:
- Viewed SVN commit in a Subversion repository with no callsign.
- Added a callsign, looked at it again.
- Viewed non-SVN commits in callsign and non-callsign repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10721
Differential Revision: https://secure.phabricator.com/D15607
Summary: Ref T7303. Small modernization.
Test Plan:
- Searched by various users.
- Viewed all, reordered, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15606
Summary:
Ref T7303. Ref T7673. This implements an "auth.logout" which:
- terminates all web sessions;
- terminates the current OAuth token if called via OAuth; and
- may always be called via OAuth.
(Since it consumes an OAuth token, even a "malicious" OAuth application can't really be that much of a jerk with this: it can't continuously log you out, since calling the method once kills the token. The application would need to ask your permission again to get a fresh token.)
The primary goal here is to let Phacility instances call this against the Phacility upstream, so that when you log out of an instance it also logs you out of your Phacility account (possibly with a checkbox or something).
This also smooths over the session token code. Before this change, your sessions would get logged out but when you reloaded we'd tell you your session was invalid.
Instead, try to clear the invalid session before telling the user there's an issue. I think that ssentially 100% of invalid sessions are a result of something in this vein (e.g., forced logout via Settings) nowadays, since the session code is generally stable and sane and has been for a long time.
Test Plan:
- Called `auth.logout` via console, got a reasonable logout experience.
- Called `auth.logout` via OAuth.
- Tried to make another call, verified OAuth token had been invalidated.
- Verified web session had been invalidated.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303, T7673
Differential Revision: https://secure.phabricator.com/D15594
Summary:
Ref T7303. OAuth scope handling never got fully modernized and is a bit of a mess.
Also introduce implicit "ALWAYS" and "NEVER" scopes.
Always give tokens access to meta-methods like `conduit.getcapabilities` and `conduit.query`. These do not expose user information.
Test Plan:
- Used a token to call `user.whoami`.
- Used a token to call `conduit.query`.
- Used a token to try to call `user.query`, got rebuffed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15593
Summary:
Ref T7303. This inches toward properly-behaved cluster logout.
- Use IDs instead of PHIDs in URIs.
- Slightly more modern code.
- Fix some crumb stuff.
Test Plan: Created, edited, viewed, deleted, showed secret for, authorized, test-auth'd an application.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15592
Summary: Updates various /people/ pages for new UI and newPage
Test Plan: Review creating people, new people, sending invites, editing a profile, setting a new picture, something with LDAP
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15604
Summary: Cleans up Feed Story individual page
Test Plan: View an individual story by clicking on date.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15599
Summary: No UI updates, just swapping over to `newPage`
Test Plan: Pull up each page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, yelirekim
Differential Revision: https://secure.phabricator.com/D15601
Summary: Updates fund for new edit UI
Test Plan: Create Fund, Edit Fund
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15595
Summary: Updates Macro Audit Edit page with new UI and newPage
Test Plan: Edit Audio on macro, see new layout, save file.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15598
Summary: Brings the edit paths page in owners up to new UI
Test Plan: Edit some paths, yo.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15596
Summary: Closes T8940, recipient list in badge view should show awarder and date info. Took a first stab at how we want to make the date look, but not sure. Looks odd as it is.
Test Plan: Open badge that has awards. Each recipient in list should have a subheader such as "Awarded by ... on ..."
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8940
Differential Revision: https://secure.phabricator.com/D15590
Summary: Missed converting this page, scenario. The box was poorly formatted.
Test Plan: Create a new document that needs signed, verify box is correctly spaced and colored.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15591
Summary: Forgot a more efficient way to get badge from award
Test Plan: Badges on user profiles should still show up with awarder handle on the back of the card
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15589
Summary: Ref T8940
Test Plan: Award badge, open recipient profile page, badge should appear in badges list, and flipping the badge card should show who awarded it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, Korvin
Maniphest Tasks: T8940
Differential Revision: https://secure.phabricator.com/D15570
Summary: Converts Config to new UI, updates to `newPage`
Test Plan: Review all pages in Config, setup issues, ignore an issue, edit a config option
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15587
Summary: Updates Legalpad Manage/Edit with new UI layouts.
Test Plan: Wrote a new document with and without a preamble, edit document, sign document
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15576
Summary: Pulls everything over to two column UI and new edit pages. Removed history view and consolidated some pages.
Test Plan: New Panel, Edit Panel. New Dashboard, Edit Dashboard, View Standalone pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15588
Summary: Minor, moves to `newPage`
Test Plan: Test both pages, still work
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15582
Summary: Little straggler here, updates to `newPage`
Test Plan: Review a document, no visibile changes
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15580
Summary: Updates Conpherence pages to use `newPage`
Test Plan: View a Room, view list of joined rooms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15578
Summary: Just clearing these all out.
Test Plan: Visit channel list and log page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15577
Summary:
Ref T10709. Two issues:
- If a user sends an invalid `!command`, we can throw, which means we don't return HTTP 200. This makes Mailgun re-send the mail later.
- We don't parse headers of the modern API correctly, so the "Message-ID" failsafe doesn't work. Parse them correctly. I //believe// Mailgun's API changed at some point.
Test Plan:
This is difficult to test exhaustively in isolation. I used Mailgun's web tools to verify the format of the hook request, and faked some requests locally.
I'll keep an eye on this as it goes to production and make sure the fix is correct there.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10709
Differential Revision: https://secure.phabricator.com/D15575
Summary: Modernize and use newer UI
Test Plan: Bounce around various views.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15574
Summary: Updates various Phriction pages to match new UI
Test Plan: New Document, Edit Document, View History, Revert Change
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15572
Summary: Swap over to modern components, `newPage` and `handleRequest`.
Test Plan: `arc lint` :(
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D15571
Summary: Runs through Meta, cleaned up policies and editing email addresses to new UI
Test Plan: Set a new Email address for Maniphest, edit policies.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15569
Summary: Fixes T10704. This is just bad copy-paste -- "O" for "old" should be "N" for "new".
Test Plan:
- Followed steps on T10704.
- Applied patch.
- Marked inline done, replied, etc. No more JS errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10704
Differential Revision: https://secure.phabricator.com/D15566
Summary: Cleans up Pholio, moves to two column layout, fix some transaction inconsistencies. This moves "Image" to the MainColumn, which feels fine, but I think we'll likely want some sort of "fullscreen" option for Pholio V2 like we have on workboards perhaps.
Test Plan:
New Mock, Edit Mock, View Mock.
{F1200450}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15564
Summary: Ref T9007
Test Plan: Navigate to "Advanced Search" in Badges, order by rarity, then by commonality. Rarest and most common badges should be ordered, respectively.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9007
Differential Revision: https://secure.phabricator.com/D15555
Summary: This doesn't hit the ambiguous case in Diffusion so it seems fine to make it more consistent.
Test Plan: Looked at a little task-o.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15561
Summary: Walks through various object, rule, create forms and transcripts in Herald. Slightly nicer looking.
Test Plan: Make rules, see rules, edit rules, see transcripts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15559
Summary: Fixes T10672. Cleaning this up myself since I was responsible for the implementation.
Test Plan: Leave a comment, Edit a badge, create a badge.
Reviewers: lpriestley, epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10672
Differential Revision: https://secure.phabricator.com/D15556
Summary: Adds basic commenting to Fund Initiatives.
Test Plan: Leave a comment, see comment.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15554
Summary: Uses BLUE_PROPERTY on Recipients box, removes redundent properties since we render the badge itself already.
Test Plan: View a badge with and without a description.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15553
Summary: [WIP] Tossing this up for safety and to read through it. Need to test, update some of the other flows. This updates everything in Auth for new UI and modern conventions.
Test Plan: Loooots of random testing, new providers, edit providers, logging out, forgot password... more coming.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15550
Summary:
[WIP] Allows awarding a badge from a user profile. Unsure of the interactions here if a user can't award any badges, or if we should just hide this.
Fixes T10688
Fixes T10318
Test Plan: Award some badges. Steal them back.
Reviewers: lpriestley, epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10318, T10688
Differential Revision: https://secure.phabricator.com/D15544
Summary:
Ref T10537. Currently, Nuance has a `NuanceRequestor` object, intended to represent the external user who created content (e.g., a GitHub account or a Twitter account or whatever).
This object is currently almost unused, and its design predates Doorkeeper. In D15541, I chose to use doorkeeper objects instead of NuanceRequestor objects to represent requestors.
I don't currently anticipate a need for such an object, given that we have Doorkeeper. If we do need it in the future for some reason, it would be fairly easy to restore it, create a requestor type which wraps a Doorkeeper object, and then migrate. Not super thrilling to do that, but not a huge mess.
`NuanceItem` still has a `requestorPHID`, but this is now a less formal object PHID instead of a more formal Requestor-object PHID, and holds a doorkeeper exeternal object PHID for GitHub events.
Test Plan:
- Grepped for `nuancerequestor`.
- Ran `bin/storage upgrade -f`.
- Grepped for `requestor`, remaining uses of this term seem reasonable/correct.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15546
Summary: This is kinda bad in terms of UI (It just makes a json of the thing and diffs that), but it's a start.
Test Plan: edit rule, create rule, add/remove/edit conditions, actions
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15542
Summary:
Ref T10538. Ref T10537. This creates PHIDs which represent GitHub users, and uses them as the actors for synchronized comments.
I've just made them Doorkeeper objects. There are three major kinds of objects they //could// possibly be:
- Nuance requestor objects.
- External account objects.
- Doorkeeper objects.
I don't think we actually need distinct nuance requestor objects. These don't really do anything right now, and were originally created before Doorkeeper. I think Doorkeeper is a superset of nuance requestor functionality, and better developed and more flexible.
Likewise, doorkeeper objects are much more flexible than external account objects, and it's nice to imagine that we can import from Twootfeed or whatever without needing to build full OAuth for it. I also like less stuff touching auth code, when possible.
Making these separate from external accounts does make it a bit harder to reconcile external users with internal users, but I think that's OK, and that it's generally desirable to show the real source of a piece of content. That is, if I wrote a comment on GitHub but also have a Phabricator account, I think it's good to show "epriestley (GitHub)" (the GitHub user) as the author, not "epriestley" (the Phabricator user). I think this is generally less confusing overall, and we can add more linkage later to make it clearer.
Test Plan:
{F1194104}
{F1194105}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537, T10538
Differential Revision: https://secure.phabricator.com/D15541
Summary: Ref T10538. This probably gets push events where GitHub does not recognize the author wrong, but I don't have any of those yet.
Test Plan: Added and ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15540
Summary: Ref T10537. Add a new content source for Nuance. Prepare for better author attribution.
Test Plan: {F1194038}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15539
Summary: Fixes T10670, for users with exclusively archived badges, user profile should show "no badges" message instead of blank box
Test Plan: Award badge to user with no badges, archive badge, user profile should show "no badges" message under badges.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10670
Differential Revision: https://secure.phabricator.com/D15538
Summary:
Ref T10537.
- Let nuance items render custom curtain panels.
- Add a custom panel linking to the imported task, if one exists.
- Actually extract comments properly.
Test Plan:
Unit tests, plus:
{F1193800}
{F1193801}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15537
Summary: Ref T10677, Awarding/revoking badge should create a feed story on homepage with badge handle recipient handles
Test Plan: Award/revoke badge, open Feed, should see story with badge link and recipient links.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10677
Differential Revision: https://secure.phabricator.com/D15534
Summary: Ref T10677, awarding/revoking a badge should create timeline entries with titles that are more clear (excludes homepage feed stories)
Test Plan: Award/revoke a badge to single or multiple users. See timeline entries that reflect those actions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10677
Differential Revision: https://secure.phabricator.com/D15533
Summary: Cleans up EditEngine, adds new layout to EditEngine and descendents
Test Plan: Test creating a new form, reordering, marking and unmarking defaults. View new forms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15531
Summary: Adds headers, new layout to edit panels on Almanac.
Test Plan: Pull up each edit panel in sandbox, save form.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15527
Summary: Ref T8996, Convert badge recipients from Edges to actual BadgeAward objects
Test Plan: Create badge, award it to recipient. Make sure adding/removing recipients works. (Still need to migrate exisiting recipients to new table and need to create activity feed blurbs)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin
Maniphest Tasks: T8996
Differential Revision: https://secure.phabricator.com/D15014
Summary:
Fixes T10385. Two issues:
- `$show_blame` and `$show_color` were improperly swapped.
- Code to hide these columns got dropped somewhere, probably in my recent-ish rewrite.
Test Plan:
- Showed/hid blame.
- Showed/hid colors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10385
Differential Revision: https://secure.phabricator.com/D15528
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary: Updates the Harbormaster UI to match the new two column everywhere else.
Test Plan: Did best I could, tested builds, plans, steps, buildables. Unable to test lint/unit locally, I need to set that up. Kick the tires for me pls. :3
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15523
Summary: Ref T10537. Show when an object is bridged to something external.
Test Plan: {F1190099}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15520
Summary:
Fixes T10665. See that task for discussion.
Because `$head_map` is not properly re-initialized for each ref we check, pushes which affect multiple branches (say, "A" and "B") can have information bleed from the first branch check to the second branch.
To trigger a problem behavior, you can push one commit which updates an existing branch, plus one commit which creates a new branch. If they process in the right order, the `$head_map` from the updated branch will bleed into the `$head_map` for the new branch and trigger an incorrect head split detection.
Test Plan:
- Pushed a set of changes which updated `branch-a` and created `branch-b`.
- Before change: improper detection of split heads.
- After change: clean push.
- Pushed a set of changes which split the head of `branch-d`.
- Correct detection of split heads.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10665
Differential Revision: https://secure.phabricator.com/D15522
Summary: Ref T10538. This makes us render better human-readable descriptions of more GitHub event types.
Test Plan: Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15516
Summary: Ref T10538. Very sloppy, but starting to sort of work. This sort of gets a piece of framework into a reasonable spot, next couple of diffs are going to be "extract comment text" and "show stuff in the UI" sorts of things.
Test Plan: {F1186726}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15511
Summary: This URI is currently a little whack.
Test Plan:
- With MFA, clicked "Edit Subscription" on a subscription.
- Clicked "Cancel".
- Before: Sent to `/phortune/phortune/edit/...`, a 404.
- After: Properly returned to subscription detail page.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15514
Summary: Moves everything I could find in Phortune to new UI layouts.
Test Plan: Tested every page I could get two, unclear how to test subscriptions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15513
Summary:
Ref T10537. Generally, when users interact with Nuance items we'll dump a command into a queue and apply it in the background. This avoids race conditions with multiple users interacting with an item, which Nuance is more subject to than other applications because it has an import/external component.
The "sync" command doesn't actually do anything yet.
Test Plan: {F1186365}
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15506
Summary: Ref T10538. This extracts and renders URIs for GitHub events so we can link to the original thing on GitHub.
Test Plan: {F1186332}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15505
Summary:
Ref T10537. This allows item types to expose item actions. Eventually these actions might be things like "promote to task", "tweet reply", "ban user forever", etc.
For now, provide a simple action which shows a raw item in a dialog.
Test Plan: {F1185573}
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15504
Summary:
Ref T10537. These are objects which are bound to some external object, like a Maniphest task which is a representation of a GitHub issue.
This doesn't do much yet and may change, but my thinking is:
- I'm putting these on-object instead of on edges because I think we want to actively change the UI for them (e.g., clearly call out that the object is bridged) but don't want every page to need to do extra queries in the common case where zero bridged objects exist anywhere in the system.
- I'm making these one-to-one, more or less: an issue can't be bridged to a bunch of tasks, nor can a bunch of tasks be bridged to a single issue. Pretty sure this makes sense? I can't come up with any reasonable, realistic cases where you want a single GitHub issue to publish to multiple different tasks in Maniphest.
- Technically, one type of each bridgable object could be bridged, but I expect this to never actually occur. Hopefully.
Test Plan: Ran storage upgrade, loaded some pages.
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15502
Summary: This is failing locally for me, set to getViewer and pull up cart.
Test Plan: View cart with a description.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15509
Summary: Ref T9456. This makes everything work, except that CircleCI doesn't fetch tags which are not ancestors of branch heads.
Test Plan: Ran passing builds through CircleCI.
Reviewers: chad
Reviewed By: chad
Subscribers: dpaola2, JustinTulloss
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14288
Summary: Ref T9456. Some rough edges and we can't complete the build yet since I haven't written a webhook, but this mostly seems to be working.
Test Plan:
- Ran this build on some stuff.
- Ran a normal HTTP step build to make sure I didn't break that.
{F880301}
{F880302}
{F880303}
Reviewers: chad
Reviewed By: chad
Subscribers: JustinTulloss, joshma
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14286
Summary: Ref T9456. This is just a convenience type for things like API tokens, to make it harder for users to make mistakes and keep SSH keys out of the dropdown for "choose your API token".
Test Plan: {F879820}
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14284
Summary: Fixes T10648. This was goofed and always did a meaningless no-op slice -- I mucked it up while doing the disabled project stuff elsewhere.
Test Plan:
- Tagged something with 5 projects.
- Saw the list sliced to 4 (the limit) with "...".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10648
Differential Revision: https://secure.phabricator.com/D15508
Summary:
Fixes T10646. When you load the page or click "New Condition" or "New Action", we try to add a condition and action with some default values.
Currently, the logic just sets everything to `null` or `'default'`. This technically works in Safari, but is less successful in Chrome. (I think Safari prevents you from picking an invalid value.)
Instead of relying on the browser to pick the right value, set the correct value explicitly.
Test Plan:
- Created a new rule in Chrome, Safari.
- Added fields and conditions in Chrome, Safari.
- Edited existing rules in Chrome, Safari.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10646
Differential Revision: https://secure.phabricator.com/D15507
Summary: Updates Spaces to new two column layout
Test Plan: Create a space, edit a space
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15503
Summary: Fixes T10234. This usage is unusual, out of date, and has some bad interactions with engines and custom rules.
Test Plan:
- Added `CustomInlineCodeRule` from P1129 as an extension rule.
- Put a custom `<code> ... </code>` block in a Maniphest task description.
- Saw fatal as described in task; applied change; saw rule work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10234
Differential Revision: https://secure.phabricator.com/D15501
Summary:
D9087 adds a nice typeahead but breaks the existing regex
search by quoting the pattern. Ideally, this change won't break the
typeahead, which as far as I can tell doesn't use the `pattern`
argument.
Test Plan:
Not yet.
RFC as to whether this change makes sense, will fix my local setup and resend if so.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15500
Summary: Ref T7789. Implement proper detection for read-only requests. Previously, we assumed every request was read/write and required lots of permissions, but we don't need "Can Push" permission if you're only cloning/fetching/pulling.
Test Plan:
- Set push policy to "no one".
- Fetched, got clean data out of LFS.
- Tried to push, got useful error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15499
Summary: Fixes T10625.
Test Plan: Faked this locallly and it looked OK, I'll check the mail in production. :3333
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10625
Differential Revision: https://secure.phabricator.com/D15497
Summary:
- Fix spacing on InfoView inside collasped boxes
- Fix spacing on stacked PropertyLists in TwoColumn
- Fix spacing on Readmes on Tablets
- Fix unset variable on importing commits
Test Plan: Review each of the above cases.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15496
Summary: Ref T7789. This isn't the most perfect UI imaginable, but it's similar to what GitHub does and seems reasonable.
Test Plan:
{F1180271}
{F1180272}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15494
Summary: Ref T7789. Make sure these get cleaned up when a repository is destroyed.
Test Plan:
- Created a new repository.
- Pushed some LFS data to it.
- Used `bin/remove destroy` to nuke it.
- Verified the LFS stuff was cleaned up and the underlying files were destroyed (`SELECT * FROM repository_gitlfsref`, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15493
Summary:
Ref T7789. Ref T10604. This implements the `upload` action, which streams file data into Files.
This makes Git LFS actually work, at least roughly.
Test Plan:
- Tracked files in an LFS repository.
- Pushed LFS data (`git lfs track '*.png'; git add something.png; git commit -m ...; git push`).
- Pulled LFS data (`git checkout master^; rm -rf .git/lfs; git checkout master; open something.png`).
- Verified LFS refs show up in the gitlfsref table.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789, T10604
Differential Revision: https://secure.phabricator.com/D15492
Summary: I think I like this better -- but maybe right-aligned?
Test Plan:
{F1180295}
{F1180296}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15495
Summary:
Ref T7789. This implements:
- A new table to store the `<objectHash, filePHID>` relationship between Git LFS files and Phabricator file objects.
- A basic response to `batch` commands, which return actions for a list of files.
Test Plan:
Ran `git lfs push origin master`, got a little further than previously:
```
epriestley@orbital ~/dev/scratch/poemslocal $ git lfs push origin master
Git LFS: (2 of 1 files) 174.24 KB / 87.12 KB
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
```
With `GIT_TRACE=1`, this shows the batch part of the API going through.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15489
Summary:
adds the `{{PHID....}}` rule. Should mostly be useful in UI code that refers to Objects.
It doesn't add any mention links/transactions.
Test Plan: Comment with this, see email (plain + html) and comment box.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15488
Summary: This updates (all?) of Diffusion/Audit to new UI, included edit and other extra form pages. It's fairly complete but I don't know all the nooks and crannies so to speak to fully verify I didn't mess anything up.
Test Plan: Tested creating new repositories, browsing, searching, auditing. Need more eyes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15487
Summary:
Ref T7789. This builds on top of `git-lfs-authenticate` to detect LFS requests, read LFS tokens, and route them to a handler which can do useful things.
This handler promptly drops them on the floor with an error message.
Test Plan:
Here's a transcript showing the parts working together so far:
- `git-lfs` connects to the server with SSH, and gets told how to connect with HTTP to do uploads.
- `git-lfs` uses HTTP, and authenticates with the tokens properly.
- But the server tells it to go away, and that it doesn't support anything, so the operation ultimately fails.
```
$ GIT_TRACE=1 git lfs push origin master
12:45:56.153913 git.c:558 trace: exec: 'git-lfs' 'push' 'origin' 'master'
12:45:56.154376 run-command.c:335 trace: run_command: 'git-lfs' 'push' 'origin' 'master'
trace git-lfs: Upload refs origin to remote [master]
trace git-lfs: run_command: git rev-list --objects master --not --remotes=origin
trace git-lfs: run_command: git cat-file --batch-check
trace git-lfs: run_command: git cat-file --batch
trace git-lfs: run_command: 'git' config -l
trace git-lfs: tq: starting 3 transfer workers
trace git-lfs: tq: running as batched queue, batch size of 100
trace git-lfs: prepare upload: b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69 lfs/dog1.jpg 1/1
trace git-lfs: tq: sending batch of size 1
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload
trace git-lfs: api: batch 1 files
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects/batch
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\/batch\" is not supported by this server."}
trace git-lfs: HTTP:
trace git-lfs: api: batch not implemented: 404
trace git-lfs: run_command: 'git' config lfs.batch false
trace git-lfs: tq: batch api not implemented, falling back to individual
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69
trace git-lfs: api: uploading (b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69)
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\" is not supported by this server."}
trace git-lfs: HTTP:
trace git-lfs: tq: retrying 1 failed transfers
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69
trace git-lfs: api: uploading (b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69)
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\" is not supported by this server."}
trace git-lfs: HTTP:
Git LFS: (0 of 1 files) 0 B / 87.12 KB
Git LFS operation "objects" is not supported by this server.
Git LFS operation "objects" is not supported by this server.
```
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15485
Summary:
Ref T7789. This implements a (probably) usable "git-lfs-authenticate" on top of the new temporary token infrastructure.
This won't actually do anything yet, since nothing reads the tokens.
Test Plan:
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate'
phabricator-ssh-exec: Expected `git-lfs-authenticate <path> <operation>`, but received too few arguments.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate x'
phabricator-ssh-exec: Unrecognized repository path "x". Expected a path like "/diffusion/X/" or "/diffusion/123/".
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22'
Exception: Expected `git-lfs-authenticate <path> <operation>`, but received too few arguments.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22 y'
Exception: Git LFS operation "y" is not supported by this server.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22 upload'
{"header":{"Authorization":"Basic QGdpdC1sZnM6NmR2bDVreWVsaXNuMmtnNXBtbnZwM3VlaWhubmI1bmI="},"href":"http:\/\/local.phacility.com\/diffusion\/22\/new-callsign-free-repository.git\/info\/lfs"}
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15482
Summary:
Ref T10560. Reverts D15460. See that task for discussion: we dug up some more information to explain the behavior, and this key was just sort of sidestepping an analyze/cardinality estimate issue on the index.
With proper cardinality estimates it shouldn't be used, so just nuke it.
Test Plan: Ran `bin/storage adjust`, saw key drop.
Reviewers: eadler, chad
Reviewed By: chad
Maniphest Tasks: T10560
Differential Revision: https://secure.phabricator.com/D15486
Summary: Fixes T10603. This is the last of the ad-hoc temporary tokens.
Test Plan:
- Used a file token.
- Viewed type in {nav Config > Temporary Tokens}.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15481
Summary: Ref T10603. Swap these over and give them nice UI strings.
Test Plan:
- Refreshed a Twitter OAuth link.
- Unlinked and re-linked a Twitter account.
- Viewed the new type in {nav Config > Temporary Tokens}.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15480
Summary:
Ref T10603. We have a couple of sort of ad-hoc tokens, so start formalizing them. First up is MFA tokens.
Also adds a new config module panel for these.
Test Plan:
- Added MFA.
- Added MFA, intentionally fumbled the input, completed the workflow.
- Removed MFA.
- Viewed tokens, saw MFA sync tokens.
- Viewed new module config panel.
{F1177014}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15479
Summary:
Ref T10603. This makes minor updates to temporary tokens:
- Rename `objectPHID` (which is sometimes used to store some other kind of identifier instead of a PHID) to `tokenResource` (i.e., which resource does this token permit access to?).
- Add a `userPHID` column. For LFS tokens and some other types of tokens, I want to bind the token to both a resource (like a repository) and a user.
- Add a `properties` column. This makes tokens more flexible and supports custom behavior (like scoping LFS tokens even more tightly).
Test Plan:
- Ran `bin/storage upgrade -f`, got a clean upgrade.
- Viewed one-time tokens.
- Revoked one token.
- Revoked all tokens.
- Performed a one-time login.
- Performed a password reset.
- Added an MFA token.
- Removed an MFA token.
- Used a file token to view a file.
- Verified file token was removed after viewing file.
- Linked my account to an OAuth1 account (Twitter).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15478
Summary:
Ref T10603. This converts existing hard-codes to modular constants.
Also removes one small piece of code duplication.
Test Plan:
- Performed one-time logins.
- Performed a password reset.
- Verified temporary tokens were revoked properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15476
Summary:
Ref T10603. For LFS, we need to issue a new type of temporary token.
This makes the temporary token code modular so applications can add new token types without modifying the Auth application.
(I'm moving slowly here because it impacts authentication.)
Test Plan:
- Used `bin/auth recover` to get a one-time token from the CLI.
- Used "Forgot your password?" to get a one-time token from the web UI.
- Followed the web UI token to initiate a password reset, prompting generation of a password token.
- Viewed these tokens in the web UI:
{F1176908}
- Revoked a token.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15475
Summary: Fixes T10591. This was accidentally reverted in 148a50e48b, probably when resolvign a merge/rebase.
Test Plan: Will push to production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10591
Differential Revision: https://secure.phabricator.com/D15474
Summary: Updates Drydock to use two column + curtain layouts.
Test Plan: Tested what I could get to, need @epriestley to run this locally for edge cases.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D15467
Summary:
First pass at converting Differential, I likely have some buggy-poos but thought I'd toss this up now in case very bad bugs present.
To do:
- Need to put status back on Hovercards
- "Diff Detail" probably needs a better design
Test Plan: Looking at lots of diffs, admittedly I dont have harbormaster, etc, running locally. Checked Diffusion for Table of Content changes on small and large commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15463
Summary: Fixes T10539. When building mail targets, we fail to preserve whether a recipient was originally "To" or "Cc", and just move everyone to "To".
Test Plan:
Added a comment to a task with a "To" user and a "Cc" user, with `metamta.placeholder-to-recipient` set and `metamta.one-mail-per-recipient` set.
Got mail with me Cc'd as the Cc'd user:
{F1172020}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10539
Differential Revision: https://secure.phabricator.com/D15465
Summary: Fixes T10546. Some day, decades from now, we can revisit this when we iterate on Phrequent. Just don't regress for no real reason in the meantime, since it's easy enough to keep it working in reasonable shape.
Test Plan: {F1169096}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10546
Differential Revision: https://secure.phabricator.com/D15461
Summary:
Ref T10563. This isn't a complete fix, but should make viewing complex inline threads a little more manageable.
This just tries to put stuff in thread order instead of in pure chronological order. We can likely improve the display treatment -- this is a pretty minimal approach, but should improve clarity.
Test Plan:
T10563 has a "before" shot. Here's the "after":
{F1169018}
This makes it a bit easier to follow the conversations.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10563
Differential Revision: https://secure.phabricator.com/D15459
Summary:
Ref T10493.
- Call this action "Unassigned" instead of "Up For Grabs", since the latter implies that it's OK for anyone to grab it, which is a social/context thing that we probably shouldn't assume.
- Show who a task was previously assigned to in the transaction.
The text is a little clunky, yell if you've got a better wording? Or maybe I'll come up with something.
Test Plan: {F1166299}
Reviewers: chad
Reviewed By: chad
Subscribers: cburroughs
Maniphest Tasks: T10493
Differential Revision: https://secure.phabricator.com/D15454
Summary:
Ref T10560. I don't fully understand what MySQL is doing here, but it looks like this key improves the problematic dataset in practice.
(It makes sense that this key helps, I'm just not sure why the two separate keys and the UNION ALL are so bad.)
This key isn't hugely expensive to add, so we can try it and see if there are still issues.
Test Plan: Ran `bin/storage adjust`, saw key added to table. Used `SHOW CREATE TABLE ...` to verify the key exists. Used `EXPLAIN SELECT ...` to make sure MySQL actually uses it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10560
Differential Revision: https://secure.phabricator.com/D15460
Summary: I think this works?
Test Plan:
i am wizard
{F1168808}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15457
Summary: Adds a date with the author name on the Authored By panel in Maniphest. A basic treatment, will see how it feels.
Test Plan: Look at a few tasks
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15456
Summary:
To improve the performance of Herald, we attempt to generate the value for each field (e.g., a task title) only once.
For most field values this is cheap, but for some (like a commit's branches) it can be quite expensive. We only want to pay this cost once, so we cache field values.
However, D12957 accidentally added a check where we bypass the cache and generate the value for every field, before reading the cache. This causes us to generate each field for every rule that uses it, plus one extra time.
Instead, use the cache for this check, too. Also allow the cache to cache `null`, since it can be expensive to generate `null` even though the value isn't too interesting.
The value of this early hit isn't even used (we only care if it throws or not).
Test Plan:
- Wrote a rule like "if any condition matches: branches contain a, branches contain b, branches contain c".
- Put `phlog(new Exception())` in `DiffusionCommitBranchesHeraldField`.
- Before patch, saw `bin/repository reparse --herald <any commit>` compute branches three times.
- After patch, saw only one computation.
- Verified field values in the transcript view
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15451
Summary:
Ref T10538.
This is a tiny fraction of the API. GitHub has 25 primary event types; we currently partially parse 3 of them. GitHub has 17 issue event types; we currently partially parse 12.
Test Plan: Ran `arc unit`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15448
Summary:
Ref T10538. This sets up a Doorkeeper bridge for GitHub issues, and pulls issues from GitHub to create ExternalObject references.
Broadly, does nothing useful.
Test Plan: Put a `var_dump()` in there somewhere and saw it probably do something when running `bin/nuance update --item 44`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15447
Summary:
Ref T10538. The primary GitHub event activity stream does not report minor events (labels, milestones, etc).
GitHub has a second, similar activity stream which does report these events (the "Issues Events API").
Use two separate cursors: one consumes the primary stream; the second consumes the events stream.
One possible issue with this is that we may write events in a different order than they occurred, so GitHub shows "comment, label, close" but we show "comment, close, label" or similar. This is probably OK because the secondary API doesn't seem to have any very important events (e.g., it's probably fine if label changes are out-of-order), but we can conceivably put some buffer stage in between the two if it's an issue.
Test Plan: {F1164894}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15446
Summary: Fixes T10545. Converts layout to two column.
Test Plan: Review a few project manage pages, see new layout and flag ability.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10545
Differential Revision: https://secure.phabricator.com/D15450
Summary: Ref T10545, this brings flags back? and converts the layout to two column w/curtain
Test Plan: View a few manage pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10545
Differential Revision: https://secure.phabricator.com/D15449
Summary:
Ref T10537. This adds an update daemon for pulling item data (e.g., figuring out who the author of a GitHub comment is) and routing items (e.g., sending them to a queue or applying them directly to a task).
Also adds `bin/nuance update --item X` for doing this manually for debugging.
And adds item types, for specializing item behavior. Previously, sources completely dictated item behavior, but I think we want something a little more flexible.
Test Plan:
- This still does nothing.
- Ran `bin/nuance update --item 15`.
- Saw an item route to a default queue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15441
Summary: This inverts colors and icons a bit, so they're not as harsh. So instead of a dark green item with white icon, its now light green with a dark green icon. I've also changed all text and comment boxes to be "grey" visually to separate out the UI from converation/actions. Give it a spin and let me know how this feels. I still need to update the comment UI.
Test Plan:
UIExamples, lots of various tasks and diffs.
{F1163837}
{F1163839}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15442
Summary:
Ref T10537. Ref T10538. This polls the GitHub events API and creates Nuance items from the raw data.
It does nothing useful with them.
Test Plan:
- Polled GitHub.
- Saw some items get created.
- X-Poll-Interval seemed to work.
- ETag seemed to work.
- Recognizing when we hit items we've already seen seemed to work.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537, T10538
Differential Revision: https://secure.phabricator.com/D15440
Summary: Cleaner UI, moved visit to be button.
Test Plan: Make a phurl about cats, click on it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15385
Summary:
Ref T10537. More infrastructure:
- Put a `bin/nuance` in place with `bin/nuance import`. This has no useful behavior yet.
- Allow sources to be searched by substring. This supports `bin/nuance import --source whatever` so you don't have to dig up PHIDs.
Test Plan:
- Applied migrations.
- Ran `bin/nuance import --source ...` (no meaningful effect, but works fine).
- Searched for sources by substring in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15436
Summary:
Ref T10537. Some sources (like the future "GitHub Repository" source) need to poll remotes.
- Provide a mechanism for sources to emit import cursors.
- Hook them into the trigger daemon so they'll fire periodically.
- Provide some storage.
This diff does nothing useful or interesting, and is pure infrastructure.
Test Plan:
- Ran `bin/storage upgrade -f`, no adjustment issues.
- Poked around Nuance.
- Ran the trigger daemon, verified it didn't crash and checked for Nuance stuff to do.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15435
Summary: Ref T10537. Converts sources to EditEngine.
Test Plan:
- Created a new source.
- Edited an existing source.
- Submitted a complaint with the complaint form.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15434
Summary: See D15432. There, we can use this test to check if the user //could// reassign the task by using "Edit Form" or the stacked actions, so any dedicated "claim" element is consistent with the other permissions.
Test Plan:
- Added a `var_dump($can_reassign)` after the call.
- Saw `true`.
- Edited the edit form, locked and disabled "Assigned To".
- Saw `false`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15433
Summary: Ref T10537. Update queue editing to use EditEngine.
Test Plan:
- Created a new queue.
- Edited an existing queue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15431
Summary: Ref T10537. Minor updates to simplify and modernize these codepaths.
Test Plan: Searched for queues and sources.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15429
Summary: Ref T10537. Minor cleanup of controllers to be more modern / work better on mobile.
Test Plan: Browsed all queue / source pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15428
Summary: Ref T10093. Changes must be pushed to staging before they can be landed from the web.
Test Plan:
Changes must be pushed to staging before they can be landed from the web.
{F1161909}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10093
Differential Revision: https://secure.phabricator.com/D15427
Summary:
By default, `bin/phd debug` activates `--trace`, which is incredibly verbose.
Instead, be moderately verbose by default, and only include tracing if `--trace` was passed to `bin/phd debug`.
See also D15422.
Test Plan:
- Ran `bin/phd debug task`, got moderate amount of most useful debug output.
- Ran `bin/phd debug task --trace`, got very verbose, detailed low-level debug output.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15423
Summary: Ref T10093. Show better errors when a commit fails because it has already been merged and when a fetch fails because the ref isn't present in the remote.
Test Plan:
{F1160794}
{F1160795}
Reviewers: chad
Reviewed By: chad
Subscribers: michaeljs1990, yelirekim
Maniphest Tasks: T10093
Differential Revision: https://secure.phabricator.com/D15420
Summary: Moves over everything except Maniphest, which has some special behavior.
Test Plan:
- Viewed a badge.
- Viewed a calendar event.
- Viewed a countdown.
- Viewed a Fund initiative.
- Viewed a Herald rule.
- Viewed a macro.
- Viewed an application.
- Viewed an owners package.
- Viewed a credential.
- Viewed a Ponder question.
- Viewed a poll.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15416
Summary: Moves Maniphest over, and allows application to provide ad-hoc panels more easily.
Test Plan: {F1160591}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15417
Summary: Convert Almanac interfaces to Curtain views.
Test Plan: Viewed Services, Bindings, Devices, Namespaces and Networks.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15415
Summary:
This opens up the new action column to have specialized rendering and behavior. Briefly:
- Converted applications (right now, only Paste) render a `CurtainView` to build the column content.
- This view uses new extensions to build panels (projects, subscribers, tokens).
- The panel extension code and rendering can be changed without breaking old stuff.
Minor changes:
- Token awards now load their tokens, for consistency/simplicity.
- Removed the rest of the "fork of" / "forked from" UI in Paste -- I essentially removed these features a while ago, and no one has complained.
Test Plan:
UI is a bit rough, but works, and it's going to get changed now anyway:
{F1160550}
{F1160551}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15414
Summary:
Two minor changes here:
- Replace `get/setUser()` with `get/setViewer()` for consistency with everything else.
- `getViewer()` now throws if no viewer is set. We had a lot of code that either "should" check this but didn't, or did check it in an identical way, duplicating work. In contrast, very little code checks for a viewer but works if one is not present.
Test Plan:
- Grepped for `->user`.
- Attempted to fix all callsites inside `*View` classes.
- Browsed around a bunch of applications, particularly Calendar, Differential and Diffusion, which seemed most heavily affected.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15412
Summary:
Every caller returns `true`. This was added a long time ago for Projects, but projects are no longer subscribable.
I don't anticipate needing this in the future.
Test Plan: Grepped for this method.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15409
Summary: Clean up owners a bit, move to two columns.
Test Plan:
Review a package, edit paths, remove all paths. Archive.
{F1139351}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15388
Summary:
Fixes T10519. Two issues:
First, the acting user wasn't explicitly included in the mail. This usually didn't matter, but could matter if you unsubscribed and then interacted.
Second, we had some logic which tried to hide redundant "added inline comment" transactions, but could hide them inappropriately. In particular, if another action (like a subscribe) was present in the same group, we could hide the inlines because of that other transaction, then //also// hide the subscribe. This particular issue is likely an unintended consequence of hiding self-subscribes.
Instead of hiding inlines if //anything else// happened, hide them only if:
- there is another "added a comment" transaction; or
- there is another "added an inline comment" transaction.
This prevents the root issue in T10519 (incorrectly hiding every transaction, and thus not sending the mail) and should generally make behavior a little more consistent and future-proof.
Test Plan:
- Submitted //only// an inline comment on a commit I had not previously interacted with.
- Before patch: no mail was generated (entire mail was improperly hidden).
- After patch: got some mail with my comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10519
Differential Revision: https://secure.phabricator.com/D15407
Summary: Sets a header icon, makes "Details" not show if empty, simplifies title.
Test Plan: Review a few Macro pages for changes with and without audio.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15406
Summary: Converts the meta applications application view layout to two column
Test Plan: click through "Configure" on each application, set up some emails. uninstall Phrequent
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15405
Summary: Moves over to the new layout. Fixes T10521
Test Plan: Make a binding, view page, add some properties.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10521
Differential Revision: https://secure.phabricator.com/D15404
Summary: Switch to new method.
Test Plan: Hover over task, see tag in correct place.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15403
Summary: Gives a bit more flexibility to add anything to the right side of PHUIHeaderView.
Test Plan: Test Maniphest, Workboards, Project Home, Differential. Grep for `addActionIcon` use. Fixes T10518
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10518
Differential Revision: https://secure.phabricator.com/D15402
Summary: Reworks Maniphest into a two column view. Moves priority and color to header, assignee to sidebar. quest points to header, and author to gutter. may be some confusion since priority only displays on open tickets.
Test Plan: with and without description, custom fields, points, tablet, mobile and desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15396
Summary:
Ref T10246. Drydock still has a few outstanding issues, but it's mostly minor UI stuff and the documentation has reasonable caveats about it.
Broadly, I don't expect to make any major changes to Drydock in the future (i.e., all the fundamentals seem sound at this point) and it doesn't have any major technical debt or, like, obsolete APIs or anything.
Test Plan: Saw "Drydock" as not-a-prototype in Applications.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10246
Differential Revision: https://secure.phabricator.com/D15401
Summary: Fixes T10449. Almanac doesn't do a whole lot for the average user, but is in good shape technically and works well, and exposing it in the cluster won't let installs destroy themselves now.
Test Plan: Re-read documentation; grepped for `TODO` (there are a couple, but reasonable to push off); browsed around all the UI things (new two-column looks great), called API methods.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10449
Differential Revision: https://secure.phabricator.com/D15400
Summary: Ref T10449. Modernize the AlmanacDevice code a bit.
Test Plan:
- Created a device.
- Edited a device.
- Listed devices.
- Viewed a device.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10449
Differential Revision: https://secure.phabricator.com/D15399
Summary: Ref T10449. This modernizes the service creation/editing flow and updates the list view code a little bit.
Test Plan:
- Created a service.
- Edited a service.
- Browsed services.
- Hit policy exception for editing cluster services with no permission.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10449
Differential Revision: https://secure.phabricator.com/D15398
Summary: Fix an issue where you've already answered, moved the summary section.
Test Plan: Review an answer with a wiki that i've already answered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15395
Summary: Ref T10457. Use modern controller and UI tech to build the list view and actions.
Test Plan:
- Viewed operation list.
- Viewed operation detail.
- Checked menus on mobile.
{F1139757}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15393
Summary:
Ref T10457.
- Let blueprints be tagged so you can search and annotate them a little more easily.
- Give each blueprint type an optional icon to make things a little easier to parse visually.
Test Plan:
- Tagged blueprints.
- Searched by tags.
- Looked at nice little icons.
{F1139712}
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15392
Summary:
Ref T10457. Fixes T10024. This primarily just modernizes blueprints to use EditEngine.
This also fixes T10024, which was an issue with stored properties not being flagged correctly.
Also slightly improves typeaheads for blueprints (more information, disabled state).
Test Plan:
- Created and edited various types of blueprints.
- Set and removed limits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10024, T10457
Differential Revision: https://secure.phabricator.com/D15390
Summary:
Ref T10457. The ngram indexing seems to be working well; extend it into Drydock.
Also clean up the list controller a little bit.
Test Plan:
- Ran migrations.
- Searched for blueprints by name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15389
Summary: I kinda like these to differentiate the headers and different object types. Somethings duplicitive, but helps orient the clean header a bit.
Test Plan: Review each in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15394
Summary: Fixes T10474. This had the same root cause as T10024 -- a missing call to `didSetValueFromStorage()` because of the way the fields work.
Test Plan:
- Edited a text panel before change, without changing text: got silly transaction.
- Made change, edited text panel without changing text, no transaction.
- Made a real edit, got a good transaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10474
Differential Revision: https://secure.phabricator.com/D15391
Summary: Fixes T10504. The "Create Blog" buttons weren't generated by EditEngine, but should be, so that the UI and policies are in sync.
Test Plan:
- Viewed blog list as user with and without permission to create blogs. Saw correct button state.
- Tried to create blogs, saw correct result.
- Viewed empty state of home, clicked "New Blog" buttons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10504
Differential Revision: https://secure.phabricator.com/D15384
Summary: Updates the Fund application to use a two column layout
Test Plan: Make an initiative.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15383
Summary: Simplifies building pages a little more, adds a helper method to just add a property section to the main column automatically above other content.
Test Plan: Review Ponder, Herald, Passphrase, Countdown.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15377
Summary: Updates Almanac to the new layout, adds some header icons for interest.
Test Plan: Click on all the different almanac pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15381
Summary:
Ref T5822.
- After a log is closed, compress it if possible.
- Provide `bin/harbormaster archive-logs` to make it easier to change the storage format of logs.
Test Plan:
- Ran `bin/harbormaster archive-logs` on a bunch of logs, compressing and decompressing them without issues (same hashes, same decompressed size across multiple iterations).
- Ran new builds, verified logs were compressed after they closed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5822
Differential Revision: https://secure.phabricator.com/D15380
Summary: Ref T5822. This will make it easier to compress and archive chunks without needing to hold them in memory.
Test Plan: Ran a build, looked at some logs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5822
Differential Revision: https://secure.phabricator.com/D15378
Summary:
Ref T10457. Currently, this table is an ad-hoc table, but can easily be turned into a normal table.
This will make iterating over log chunks to compress and archive them easier.
Test Plan: Viewed logs, ran `bin/storage adjust` with no issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15376
Summary:
Ref T10457. Currently, every `append()` call necessarily generates queries, and these queries are slightly inefficient if a large block of data is appended to a partial log (they do about twice as much work as they technically need to).
Use `PhutilRope` to buffer `append()` input so the logic is a little cleaner and we could add a rule like "flush logs no more than once every 500ms" later.
Test Plan:
- Ran a build, saw logs.
- Set chunk size very small, ran build, saw logs, verified small logs in database.
{F1137115}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15375
Summary:
Ref T5822. This prepares for inline compression and garbage collection of build logs.
This reduces the API surface area and removes a log from the "wait" step that would just log a message every 15 seconds. (If this is actually useful, I think we find a better way to communicate it.)
Test Plan:
Ran a build, saw a log:
{F1136691}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5822
Differential Revision: https://secure.phabricator.com/D15371
Summary:
Rolls out a new "Object Page" design with PHUITwoColumnView. This is reasonably polished, but wanted to post it up for you now for feedback before chasing down minor bugs. This implements TwoColumn in the following applications:
- Ponder
- Paste
- Slowvote
- Countdown
- Projects
- Profile
- Passphrase
This helped track down display issues and inconsistencies and make sure the layout was flexible for different pages.
Test Plan:
Test each of the applications on mobile, tablet, and desktop breakpoints.
{F1135705}
{F1135706}
{F1135707}
{F1135708}
{F1135709}
{F1135710}
{F1135711}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15366
Summary:
Ref T10457. Skipped tests are almost always well-behaved (e.g., `testWindows()`, but the test is running on Linux) and not interesting, and we do not expect well-written, solid systems to necessarily have 0 skips.
Although skips //could// indicate that you have missing dependencies on a build server, and thus be a bit interesting, I think they almost always indicate that a particular test is not expected to run in the current environment.
If we wanted to tackle this problem in granular detail, we could eventually add a "Missing" status or similar which would serve as "a skip you //could// reasonably fix in this environment", but I don't think that's too interesting.
Test Plan:
Here's an example of a build result with skips: B10875
{F1136511}
I think this is clearer as "Passed", as this is the expected production state of the build.
Locally, looked at some builds.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15369
Summary: Fixes T10478.
Test Plan: Paged projects by "Created" without errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10478
Differential Revision: https://secure.phabricator.com/D15367
Summary: Ref T10457. This gives unit test results a more first-class treatment in the Differential UI, and consolidates some rendering code.
Test Plan:
Before:
{F1135536}
After:
{F1135537}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15365
Summary:
Ref T10457. When tests fail, it currently takes several clicks to figure out //why// they failed.
In this project, map rebuilds and `liberate` are fairly common failure conditions, but verifying that they were the root issue requires jumping into a build, then scrolling through a log.
Instead, display details if they're available.
Test Plan:
Before:
{F1135453}
After:
{F1135454}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9951, T10457
Differential Revision: https://secure.phabricator.com/D15363
Summary: Ref T10457. These lack color and iconography and are difficult to parse. Make them easier to read.
Test Plan:
Before:
{F1135396}
After:
{F1135399}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15362
Summary: Ref T10457. This is mostly just for consitency, but I imagine it will make managing large/complex build processes easier, and if we support Herald rules it would eventually let you write "Build plan's tags include [whatever]" to apply behavior to a group of plans.
Test Plan: {F1133107}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15360
Summary: Ref T10457. Allow build plans to be queried by name.
Test Plan:
- Searched for plans by name.
- Renamed a plan, searched for new name.
{F1133085}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15359
Summary:
Fixes T10466. Currently, clicking "Disable Mail" or "Enable Mail" on a project toggles an edge, but it gets a default "added an edge" story and transaction record.
These are confusing, useless and not interesting, so just hide them.
Test Plan:
- Before patch: clicked enable/disable mail, saw "added an edge" / "removed an edge" stories in feed and project history.
- After patch: clicked enable/disable mail, saw nothing in feed or project history.
- (Note that this patch is not retroactive for feed, so already-published stories won't unpublish.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10466
Differential Revision: https://secure.phabricator.com/D15361
Summary:
Ref T10457.
- Use EditEngine for Build Plans.
- Fix some minor issues with crumbs being inconsistent.
- Fix some minor issues with mobile menus not being consistent/available.
Test Plan:
- Created and edited build plans.
- Poked around in mobile width, verified mobile menu had the right stuff in it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15357
Summary:
Fixes T10011.
- Modernize searching for buildables.
- Prepare for `harbormaster.buildable.search`.
- Allow users to query by status (see T10011).
- Collapse the four weird "commit / diff / revision / repository" fields into two slightly less weird fields with more UI hinting?
Test Plan: {F1131918}
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10011
Differential Revision: https://secure.phabricator.com/D15356
Summary:
Ref T10457. This makes diffs/revisions show the revision as the buildable title, and commits show the commit as the title.
Previously, the title was "Buildable X".
Also makes icons/colors/labels more consitent.
Test Plan: {F1131885}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15355
Summary:
Ref T10457. We currently have these weird, out-of-place methods on Harbormaster queries that just load handles. These were written before HandlePool, and HandlePool is now more convenient, simpler, and more efficient.
Drop this stuff in favor of using handle pools off `$viewer`.
Test Plan: Looked at buildable list, looked at buildable detail, grepped for removed methods.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15354
Summary: Fixes T9614. This is kind of silly, but stop users from fighting turf wars over build resources or showing up on an install and just aborting a bunch of builds for the heck of it.
Test Plan:
- Restarted / paused / aborted / etc builds.
- Tried to do the same for builds I didn't have edit permission on the build plan for, got errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9614
Differential Revision: https://secure.phabricator.com/D15353
Summary: Fixes T10458. These steps are obsolete and have not worked since the last updates to Drydock. They may eventually return in some form, but get rid of them for now since they're confusing.
Test Plan:
- Created a build plan with these steps.
- Removed these steps.
- Verified the build plan showed that the steps were invalid, and that I could delete them.
- Deleted them.
- Added new steps, no obsolete steps were available for selection.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10458
Differential Revision: https://secure.phabricator.com/D15352
Summary:
Ref T10449. Currently, we store classes (like "AlmanacClusterRepositoryServiceType") in the database.
Instead, store types (like "cluster.repository").
This is a small change, but types are a little more flexible (they let us freely reanme classes), a little cleaner (fewer magic strings in the codebase), and a little better for API usage (they're more human readable).
Make this minor usability change now, before we unprototype.
Also make services searchable by type.
Also remove old Almanac API endpoints.
Test Plan:
- Ran migration, verified all data migrated properly.
- Created, edited, rebound, and changed properties of services.
- Searched for services by service type.
- Reviewed available Conduit methods.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10449
Differential Revision: https://secure.phabricator.com/D15346
Summary:
Fixes T9762. Ref T10246.
**Disabling Bindings**: Previously, there was no formal way to disable bindings. The internal callers sometimes check some informal property on the binding, but this is a common need and deserves first-class support in the UI. Allow bindings to be disabled.
**Deleting Interfaces**: Previously, you could not delete interfaces. Now, you can delete unused interfaces.
Also some minor cleanup and slightly less mysterious documentation.
Test Plan: Disabled bindings and deleted interfaces.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T9762, T10246
Differential Revision: https://secure.phabricator.com/D15345
Summary:
Fixes T6741. Ref T10246. Broadly, we want to protect Almanac cluster services:
- Today, against users in the Phacility cluster accidentally breaking their own instances.
- In the future, against attackers compromising administrative accounts and adding a new "cluster database" which points at hardware they control.
The way this works right now is really complicated: there's a global "can create cluster services" setting, and then separate per-service and per-device locks.
Instead, change "Can Create Cluster Services" into "Can Manage Cluster Services". Require this permission (in addition to normal permissions) to edit or create any cluster service.
This permission can be locked to "No One" via config (as we do in the Phacility cluster) so we only need this one simple setting.
There's also zero reason to individually lock //some// of the cluster services.
Also improve extended policy errors.
The UI here is still a little heavy-handed, but should be good enough for the moment.
Test Plan:
- Ran migrations.
- Verified that cluster services and bindings reported that they belonged to the cluster.
- Edited a cluster binding.
- Verified that the bound device was marked as a cluster device
- Moved a cluster binding, verified the old device was unmarked as a cluster device.
- Tried to edit a cluster device as an unprivileged user, got a sensible error.
{F1126552}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6741, T10246
Differential Revision: https://secure.phabricator.com/D15339
Summary: Evidently I only tested adding a question, not an answer. Properly set the getter. Also, fixed some header spacing.
Test Plan: Add a question, add an answer. See everything work, proper spacing.
Reviewers: epriestley, avivey
Reviewed By: avivey
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15341
Summary: Full new UI, testing some upcoming treatments for consideration in other View controllers. Small tweaks to allow PHUITwoColumnView to have fixed and fluid width, and let TransactionCommentView go fullWidth.
Test Plan:
Tested a number of Ponder cases, New Question, with and without summary, with and without answers, with and without comments. Mobile, Tablet, and Desktop layouts. Verify Project and Profile UI's still in tact.
{F1120961}
{F1120962}
{F1120963}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15315
Summary:
Ref T10349.
- Don't show subproject columns on "Manage Board".
- Fix "Edit Column" for milestone columns (allows you to set points, but not rename).
Test Plan:
- Viewed "Manage Board" on a project with subprojects.
- Edited a milestone column and set a point limit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15338
Summary: Fixes T10432. I missed these in making properties non-default.
Test Plan: Diffusion now works again in a cluster configuration.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10432
Differential Revision: https://secure.phabricator.com/D15337
Summary: Fixes T10413. I accidentally hid these //everywhere//, but only intended to hide them on workboards.
Test Plan:
- Viewed a workboard, saw un-archived projects only.
- Viewed a task detail page, saw archived and un-archived projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10413
Differential Revision: https://secure.phabricator.com/D15335
Summary:
Fixes T10414. I think this sorted by name at one time (the `asort()`) but then I probably added "Space SX" in front of it. Or I just got this wrong from the beginning.
Instead, sort by space name.
Test Plan: {F1126034}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10414
Differential Revision: https://secure.phabricator.com/D15334
Summary:
Fixes T10426. When the owner of a task changes, we try to add the old owner to CC so they're kept in the loop.
Currently, we do this unconditionally. This can add the owner as a subscriber when someone didn't change anything, which is confusing.
Instead, only do this if the owner actually changed.
Test Plan:
- With "A" as owner, edited task and saved.
- Before patch, A was added as subscriber.
- After patch, A not added.
- With "A" as owner, changed owner to "B" and saved.
- Both before and after patch, "A" is added as a subscriber.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10426
Differential Revision: https://secure.phabricator.com/D15333
Summary: Fixes T10411. Ref T10246. There are probably still some rough edges with this, but replace the old-school endpoints with modern ones so we don't unprototype with deprecated stuff.
Test Plan:
- Made a bunch of calls to the new endpoints with various constraints/attachments.
- Created and edited services, devices, interfaces, bindings, and properties on everything.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10246, T10411
Differential Revision: https://secure.phabricator.com/D15329
Summary: Fixes T10409. Long term need to build a proper "PageEngine" of sorts for layouts not needing special magic. For now this just affects a few applications.
Test Plan: View Diffusion, Phriction, Phame, Legalpad, Diviner.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10409
Differential Revision: https://secure.phabricator.com/D15328
Summary:
Ref T10411. This cleans up / modernizes things and lets me get an `almanac.network.edit` API in the future.
This is mostly straightforward, except that Services have an extra "choose type" screen in front of them.
Test Plan:
- Created and edited Almanac networks, services, and devices.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10411
Differential Revision: https://secure.phabricator.com/D15326
Summary:
Fixes T10410. Immediate impact of this is that you can now actually delete properties from Almanac services, devices and bindings.
The meat of the change is switching from CustomField to EditEngine for most of the actual editing logic. CustomField creates a lot of problems with using EditEngine for everything else (D15326), and weird, hard-to-resolve bugs like this one (not being able to delete stuff).
Using EditEngine to do this stuff instead seems like it works out much better -- I did this in ProfilePanel first and am happy with how it looks.
This also makes the internal storage for properties JSON instead of raw text.
Test Plan:
- Created, edited and deleted properties on services, devices and bindings.
- Edited and reset builtin properties on repository services.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10410
Differential Revision: https://secure.phabricator.com/D15327
Summary:
Ref T10246. Ref T6741.
When you have a namespace like "phacility.net", require users creating services and devices within it to have edit permission on the namespace.
This primarily allows us to lock down future device names in the cluster, so instances can't break themselves once they get access to Almanac.
Test Plan:
- Configured a `phacility.net` namespace, locked myself out of it.
- Could not create new `stuff.phacility.net` services/devices.
- Could still edit existing devices I had permission for.
- Configured a `free.phacility.net` namespace with more liberal policies.
- Could create `me.free.phacility.net`.
- Still could not create `other.phacility.net`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6741, T10246
Differential Revision: https://secure.phabricator.com/D15325
Summary:
Ref T6741. Ref T10246.
Root problem: to provide Drydock in the cluster, we need to expose Almanac, and doing so would let users accidentally or intentionally create a bunch of `repo006.phacility.net` devices/services which could conflict with the real ones we manage.
There's currently no way to say "you can't create anything named `*.blah.net`". This adds "namespaces", which let you do that (well, not yet, but they will after the next diff).
After the next diff, if you try to create `repo003.phacility.net`, but the namespace `phacility.net` already exists and you don't have permission to edit it, you'll be asked to choose a different name.
Also various modernizations and some new docs.
Test Plan:
- Created cool namespaces like `this.computer`.
- Almanac namespaces don't actually enforce policies yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6741, T10246
Differential Revision: https://secure.phabricator.com/D15324
Summary: Ref T6741. Ref T10246. This is largely modernization, but will partially support namespace locking in Almanac.
Test Plan:
Searched for Almanac networks by name substring.
{F1121740}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6741, T10246
Differential Revision: https://secure.phabricator.com/D15322
Summary: Ref T10246. Build an ngram index for Almanac services, and use it to support improved search.
Test Plan: {F1121725}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10246
Differential Revision: https://secure.phabricator.com/D15321
Summary:
These trip me up every time because Differential has:
> Comment, Accept, Request Changes, Resign, Commandeer, Add Reviewers, Add Subscribers
while audits currently show:
> Comment, Add Subscribers, Add Auditors, Accept, Raise Concern, Resign
Now they're more or less in the same order which helps with muscle memory.
Test Plan: Careful inspection.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15323
Summary:
Fixes T10205. Ref T10246. Previously, the issue was that the result set was not ordered, so "More Results" would not have been able to work in a reasonable way if there were more than 100 matching interfaces.
You would have seen 100 interfaces more or less at random, then clicked "more" and gotten 100 more random interfaces.
Now, you would see 100 "a" interfaces, then click more to get the next 100 alphabetical interfaces (say, "b" and "c" interfaces).
Test Plan:
- Clicked browse when binding an interface.
- Got a browse dialog.
- Artificially set query limit to 1, paged through "local" interfaces in an ordered, consistent way.
{F1121313}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10205, T10246
Differential Revision: https://secure.phabricator.com/D15320
Summary:
Ref T10205. Ref T10246. This is general modernization, but also supports fixing the interface datasource in T10205.
- Update Query.
- Update SearchEngine.
- Use an ngrams index for searching names efficiently.
Test Plan:
- Ran migrations.
- Searched Almanac devices by name.
- Created a new device, searched for it by name.
{F1121303}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10205, T10246
Differential Revision: https://secure.phabricator.com/D15319
Summary: Not terribly useful. Also removed close your stuff reminder.
Test Plan: View question I asked and strangers question. Both layout more normal like.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15312
Summary: Ref T10394. Currently, these rules are only active if the Macro application is installed. Instead, install them unconditionally.
Test Plan:
- Used `{icon camera}` with Macro installed and uninstalled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10394
Differential Revision: https://secure.phabricator.com/D15311
Summary: Swapping out to PHUIDocumentProView to remove all calls to PHUIDocumentView.
Test Plan: Review the Phabricator Readme.MD in Diffusion
Reviewers: epriestley, avivey
Reviewed By: avivey
Subscribers: avivey, Korvin
Differential Revision: https://secure.phabricator.com/D15308
Summary:
WMF ran into this after their update. Here's the setup:
- When you enable Spaces, we leave all existing objects set to `null`, which means "these belong to the default space". This is so we don't have to go update a trillion objects.
- New objects get set to the default space explicitly (`PHID-SPCE-...`) but older ones stay with `null`.
- If you edit an older object (like a task) from the time before Spaces, //and// the form doesn't have a Visbility/Spaces control, we would incorrectly poplate the value with `null` when the effective value should be the default space PHID.
- This caused a "You must choose a space." error in the UI.
Instead, populate the control with the effective value instead of the literal database value. This makes the edit go through cleanly.
Also add a note about this for future-me.
Test Plan:
- Disabled "Visibility" control in task edit form.
- Edited an old task which had `null` as a `spacePHID` in the database.
- Before patch: UI error about selecting a Space.
- After patch: edit goes through cleanly.
Reviewers: chad, 20after4
Reviewed By: chad, 20after4
Subscribers: 20after4, aklapper
Differential Revision: https://secure.phabricator.com/D15306
Summary:
Ref T4245. This could still use a little UI smoothing, but:
- Don't require a callsign on the create flow (you can add one later in "Edit Basic Information" if you want).
- Allow existing callsigns to be removed.
Test Plan:
- Created a new repository with no callsign.
- Cloned it; pushed to it.
- Browsed around Diffusion a bunch.
- Visited a commit URI.
- Added a callsign to it.
- Removed the callsign again.
- Referenced it with `R22` in remarkup.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15305
Summary:
Ref T4245. This is a prelude to removing them from the "create" screen.
Currently, if you try to delete the callsign you get an unceremonious database error, but the next diff (or maybe two) will permit that, so I didn't put any "this is required yada yada" text in.
This could also maybe use some big flashing warning lights and a "if you edit this, all your URIs break" but I'll save that for later.
Test Plan: Changed the callsign for a repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15304
Summary: Ref T4245. When creating new repositories, set a default local path based on the repository ID instead of callsign.
Test Plan:
- Created a new repository.
- Saw it get a reasonable, ID-based local path.
- Edited a repository to make sure the `applyFinalEffects()` wasn't doing anything whacky.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15303
Summary: Ref T4245. Consolidates the URI parsing/rewriting logic so that repositories can be served from either `/diffusion/XYZ/` or `/diffusion/123/`, over both HTTP and SSH.
Test Plan:
- Pulled a Git repository by ID and callsign over HTTP and SSH.
- Pulled a Mercurial repository by ID and callsign over HTTP and SSH.
- Pulled a Subversion repository by ID and callsign over SSH (no HTTP support for SVN).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15302
Summary:
Ref T4245. Make `/diffusion/123/` work, but redirect the user to `/diffusion/XYZ/` if the repository has a callsign.
(Right now, every repository has a callsign, so this always redirects.)
Also redirect `/R123:abcdef` if the repository has a callsign.
Also also, move the Pull garbage collector somewhere more sensible.
Test Plan:
- Added test coverage.
- Visited `/diffusion/1/`, was redirected.
- Visited `/diffusion/R1:abcdef`, was redirected.
- Browsed Diffusion normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15301
Summary: Ref T10349.
Test Plan:
- Added archived and unarchived project tags to a task.
- Saw unarchived tags, only, on cards.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15297
Summary: Ref T4245. This currently accepts only callsigns; prepare it for the bright new callsign-optional world.
Test Plan:
- Ran `./bin/diviner generate --repository 1 --book src/docs/book/flavor.book --clean`, got a good result.
- Ran `./bin/diviner generate --repository 239238 --book src/docs/book/flavor.book --clean`, got an appropraite error about a bad repository identifier.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15296
Summary: Ref T4245. This has no callers.
Test Plan:
- Ran `git grep -i 'getCallsign('` and visually verified that no callers could reasonably be `DiffusionRequest` objects (there are only 23 remaining sites, and about half are `$this->...` in `PhabricatorRepository`.
- Browsed around directory/file/branch/content/diff/etc pages in Diffusion.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15295
Summary:
Ref T4245. We pass this exclusively for use by additional third-party hooks.
This is technically a backward compatibility break, but I suspect it doesn't affect anyone:
- Probably almost no one is using this (there are few reasons to, even for the tiny number of installs with custom commit hooks).
- If they are, there's a good chance the PHID will work anyway, since nearly all scripts and Conduit methods will accept it in place of a callsign now, and if it's in logging or debugging code the PHID is a reasonable substitute
- Even if it doesn't just keep working, the break should be very obvious in most reasonable cases.
I'll call this out explicitly in the changelog, though -- almost everything else will just continue working, but this is a strict compatibility break.
Test Plan:
- Ugh.
- Picked a hosted Git repo out of Diffusion.
- Went to the path on disk.
- Went into `hooks/`.
- Went into `pre-receive-phabricator.d/`.
- Wrote this hook and gave it `chmod +x`:
```name=stuff.sh
#!/bin/sh
echo $PHABRICATOR_REPOSITORY >> /tmp/stuff.log
```
- Pushed to the repository.
- Saw a PHID show up in the log:
```
$ cat /tmp/stuff.log
PHID-REPO-bqkcdp47euwnwlasrsrh
```
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15294
Summary:
Ref T4245. Two effects:
- First, let hooks work for future repositories without callsigns.
- Second, provide a better error when users push directly to hosted repositories.
Test Plan: Ran `bin/commit-hook PHID-REPO-xxx`.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15293
Summary: Working towards making PHUITwoColumnView into a page layout engine. Adds header support.
Test Plan: Use new header on Profile and Profiles. No visual changes, less duplicated code.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15292
Summary: Attempting to clean PHUIObjectBoxView up a little as well as finally being able to `addClass` on the sucker. I'm running into some issue with `addTabs` though, which on Files isn't firing.
Test Plan: Bounce around tons of screens.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15291
Summary:
Ref T10349. This capability didn't make a ton of sense when you had to be a member to watch a project and watch rules were simple, but makes more sense now.
A particular use case might be finding all the stuff you're watching so you can prune it.
Test Plan: Searched for stuff I was watching, got accurate results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15289
Summary: We're using this a little more, so I'd prefer less copy-pasta and one place to manage the UI. Maybe add a caret?
Test Plan: grep for 'Actions', test Phriction, Diviner, ect, Action Menus.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15288
Summary: Fixes T9562. We already do this for tags, but didn't have similar logic for branches. Implement that logic.
Test Plan:
- Set limit to 1, saw "More branches", clicked it, got the correct results.
- Verified that branch table with no specified commit still works properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9562
Differential Revision: https://secure.phabricator.com/D15284
Summary: Uses the background color changes to show also on the side nav. Places color on entire body so fullscreen doesn't show other body color.
Test Plan: Review various workboard colors at normal and fullscreen
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15283
Summary:
Fixes T10304. In Mercurial, we must enumerate the whole file tree. Currently, we incorrectly count files within directories (which won't be shown) toward the "100 file" limit at top level, so directories with more than 100 subpaths are truncated improperly.
This is approxiately the same as @richardvanvelzen's fix.
Test Plan: Viewed a large Mercurial repository, saw a complete directory listing.
Reviewers: chad
Reviewed By: chad
Subscribers: richardvanvelzen
Maniphest Tasks: T10304
Differential Revision: https://secure.phabricator.com/D15282
Summary: Moves all the one off object calls to PHUIRemarkupView, adds a "Document" call as well (future plans).
Test Plan: Visited most pages I could get access to, but may want extra careful eyes on this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15281
Summary:
Ref T10349. These got sort of half-weirded-up before I separated subscriptions and watching fully. New rules are:
- You can watch whatever you want.
- Watching a parent watches everything inside it.
- If you're watching "Stonework" and go to "Stonework > Masonry", you'll see a "Watching Ancestor" hint to let you know you're already watching a parent or ancestor.
Test Plan:
- Watched and unwatched "Stonework".
- Watched and unwatched "Stonework > Iteration IV".
- While watching "Stonework", visited "Iteration IV" and saw "Watching Ancestor" hint.
- Created a task tagged "Stonework > Iteration IV". Got notified about it because I watch "Stonework".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15280
Summary: Should make it simpler here to have more `rgba` rules in CSS for things like hovers, selected states. Maybe only use `rgb` colors? Color pallette probably needs an overhaul.
Test Plan: Bounce around random pages, buttons, menus. Everything appears normal.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15273
Summary:
Adds a UI for selecting background colors.
You can choose "Use Parent", which is the default, and allows you to set a color that all descendants inherit.
You can also choose "None", if a parent has a WHACKY BACKGROUND that you refuse to put up with.
Test Plan:
{F1114588}
{F1114589}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15279
Summary:
This gives us room for less-common workboard management options like "Disable Board" without overloading the menus on the main board.
Particularly, we can add background color options here without anything getting weird.
I've left "Add Column" on the main UI since I think it's common enough to leave there. We could probably move "Hide Column" to this UI in the future, though.
Test Plan: {F1114475}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15278
Summary:
Ref T10349. At HEAD, if you create a task //on a board//, it floats to the top correctly.
If you create a task elsewhere and tag it with the board, you were subject to the whims of the layout engine and it would generally end up on the bottom.
Instead, make the rules consistent so that "virtual" positions (of tasks which haven't been committed to a particular position yet) still float to the top.
Test Plan:
- Created tasks from a board.
- Created tasks from Maniphest, then looked at them on a board.
- Moved tasks around.
- In all cases, newly created tasks floated to the top.
- Sorted by natural and priority.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15276
Summary: Fixes T10361.
Test Plan:
- Created a poll with an embedded file and a mention of a task.
- Verified file was attached properly.
- Verified mention appeared on task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10361
Differential Revision: https://secure.phabricator.com/D15277
Summary: Fixes T10360. In modern code, most of the meat is automatic.
Test Plan:
- Edited view policy and edit policy from web UI.
- Viewed package, saw policy badge in header.
- Tried to edit a package as a user without permission, got appropriate disabled states and errors.
- Changed policies via Conduit.
- Tried to view a package as a user without permission.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10360
Differential Revision: https://secure.phabricator.com/D15275
Summary:
Fixes T10357.
- Show a better (more descriptive) error message when a user who can't create projects tries to create a subproject or milestone.
- Disable the subproject actions if you don't have create permission.
All this stuff was already enforced properly: this diff doesn't make any actual policy changes, just improves the UI for users who lack permission.
Test Plan:
- As an unprivileged user (no "Can Create Projects"), tried to create a subproject or milestone.
- After patch, got a disabled action, with more specific and helpful error than before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10357
Differential Revision: https://secure.phabricator.com/D15274
Summary: Makes these pages a dialog endpoint, keeping you on the Workboard when possible.
Test Plan: Create a Column, Edit a Column, visit hard page. Test letters in the points field.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15270
Summary: Cleans up the backgrounds a little on Projects/Profiles and adds a number of colour choices for Workboards.
Test Plan:
Manually add each color for testing. Test new project / profile layouts with and without properties.
{F1109325}
{F1109326}
{F1109327}
{F1109328}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15269
Summary:
Ref T10349. Columns have the same policies as the projects they belong to.
However, the current implementation just returns the policy directly. This usually works, but if the project has a policy like "Members of (This) Project", the policy filter tries to check if the viewer is a member of //the column itself//. That doesn't work, since columns don't have members. This leads to a situation where columns on "Editable By: Project Members" projects can not be edited.
Instead, return a permissive base policy and then use an extended policy to bind the column policy to the project policy.
Test Plan:
- Edited a column on an "Editable By: Members of Project" board.
- Added and ran a unit test covering this case.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15268
Summary:
Ref T10349. Instead of showing columns in "Backlog, Custom, Sprint 1, Sprint 2, Sprint 3" order, show the sprints in reverse order: 3, 2, 1.
This makes it easier to get to the new stuff, and you don't have to drag over older stuff or archive it immediately.
Trello's own meta-board for Trello development is a good example of this in the wild: older stuff goes out to the right, so you can get to the newer stuff easily:
https://trello.com/b/nC8QJJoZ/trello-development
Test Plan: Saw board in 3, 2, 1 order instead of 1, 2, 3.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15267
Summary: Ref T10350. Normally, milestone numbers and IDs have the same order, but they may not if you used the script in T10350 to artificially move a bunch of stuff around.
Test Plan: Milestones now go "1, 2, <thing I artifically moved into position 3>" on local install.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10350
Differential Revision: https://secure.phabricator.com/D15266
Summary:
Fixes T10346. You finally wrung a clue out of the reporter and I think I figured this out.
Here's the bug:
- Create a project with a workboard and subprojects/milestones.
- Create a new project, import columns from the first project.
- We incorrectly import empty columns for the subprojects/milestons.
Instead, skip proxy columns during import.
Also, allow "hide column" to continue on missing fields, so columns with no name can be hidden.
Test Plan:
- Did the stuff above.
- Workboard no longer populated with a bunch of "Unnamed Column" columns.
- Hid several "Unnamed Column" columns.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10346
Differential Revision: https://secure.phabricator.com/D15265
Summary: This just hides them, should still show on "View All".
Test Plan: Hide a Milestone, no longer see it on home. Click "View All", see all Milestones.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15264
Summary: Fixes T10347. In the long run maybe we'll try to guess this better, but for now get rid of the "Milestone X" hardcode and just show what the last one was called.
Test Plan:
- Created the first milestone for a project.
- Created the nth milestone for a project.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10347
Differential Revision: https://secure.phabricator.com/D15262
Summary:
Fixes T10285.
- If a result (like a milestone) has no primary hashtag, try to fill in a secondary hashtag.
- If we can't find any hashtag, don't return the result.
This produces these behaviors:
- By default, you can't autocomplete milestones.
- If you give one a hashtag, you can.
We might want to give milestones "special" hashtags eventually (like `#xyz/33`) but this fixes the confusing/broken behavior in the UI and we can wait for a better use case for letting you autocomplete milestones, I think.
Also, don't try to cycle hashtags when renaming milestones. This was a little inconsistent before.
Test Plan:
- Autocompleted normal projects.
- Autocompleted milestones with explicit hashtags.
- No autocomplete entry for milestones with no special hashtags.
- Used normal typeahead to get tag-less milestones to make sure I didn't break anything.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10285
Differential Revision: https://secure.phabricator.com/D15261
Summary: Fixes T10310. This replaces the "Hide Column" / "Show Column" option for milestone columns with one that archives/unarchives, and hides milestone columns when the milestone project is archived.
Test Plan:
- Hid and unhid a normal column (got normal dialogs).
- Hid and unhid a milestone column (got "archive project" dialogs, underlying project archived/unarchived, column vanished).
Reviewers: hach-que, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: jcowgar, Korvin
Maniphest Tasks: T10310
Differential Revision: https://secure.phabricator.com/D15231
Summary:
Fixes T10264. I'm reasonably confident that this is the chain of events here:
First, prior to 8269fd6e, we would ignore "Content-Encoding" when reading inbound bodies. So if a request was gzipped, we would read a gzipped body, then give `git-http-backend` a gzipped body with "Content-Encoding: gzip". Everything matched normally, so that was fine, except in the cluster.
In the cluster, we'd accept "gzip + compressed body" and proxy it, but not tell cURL that it was already compressed. cURL would think it was raw data, so it would arrive on the repository host with a compressed body but no "Content-Encoding: gzip". Then we'd hand it to git in the same form. This caused the issue in 8269fd6e: handing it compressed data, but no "this is compressed" header.
To fix this, I made us decompress the encoding when we read the body, so the cluster now proxies raw data instead of proxying gzipped data. This fixed the issue in the cluster, but created a new issue on non-cluster hosts. The new issue is that we accept "gzip + compressed body" and decompress the body, but then pass the //original// header to `git-http-backend`. So now we have the opposite problem from what we originally had: a "gzip" header, but a raw body.
To fix //this//, we could do two things:
- Revert 8269fd6e, then change the proxy request to preserve "Content-Encoding" instead.
- Stop telling `git-http-backend` that we're handing it compressed data when we're handing it raw data.
I did the latter here because it's an easier change to make and test, we'll need to interact with the raw data later anyway, to implement repository virtualization in connection with T8238.
Test Plan: See T10264 for users confirming this fix.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10264
Differential Revision: https://secure.phabricator.com/D15258
Summary:
This commit adds the `transactionID` field to manphest.gettransactions, to
satisfy the request in T10327
Test Plan: Call the `maniphest.gettransactions` endpoint, verify `transactionID` is present
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin
Differential Revision: https://secure.phabricator.com/D15250
Summary:
Fixes T6641. This allows users who have permission to edit a project to use "Save as Default" to save the current order and filter as defaults for the project.
These are per-board defaults, and apply to all users. The rationale is that I think the best default ordering/filtering depends mostly on the board, not the viewer.
This seems to align with most requests in the task, although rationale is a bit light. But, for example, it seems reasonable you might want to change the default filter to "All Tasks" on a sprint board, so you can see what's in the "Done" column.
This also fixes some minor issues I ran into:
- Herald could hit an issue while checking permissions if the project was a subproject and a non-member had a triggering rule.
- "Advanced filter..." did not prefill with the current filter.
Test Plan:
- Set default order and filter on a workboard.
- Reloaded board, saw settings stick.
- Tried to edit a board as an unprivileged user (disabled menu items, error).
- Reviewed transaction log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6641
Differential Revision: https://secure.phabricator.com/D15260
Summary:
Fixes T10343. All solutions here seem basically fine. I think adding this small bit of complexity is OK, and sorrrrt of like this behavior sometimes.
- Allow disabling this behavior per-status.
- Disable it by default for "Invalid" and "Duplicate" (I left "wontfix", since that's a resolution?).
Beyond being more flexible, I think this is slightly better?
Test Plan:
- Closed a task as invalid: no claim.
- Closed a task as resolved: claim.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10343
Differential Revision: https://secure.phabricator.com/D15257
Summary: Ref T4427. Seems fine / not egregiously broken.
Test Plan: Edited points configuration. Tried to set a bad value. Set a good value. Persued examples and help text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D15256
Summary: minor spacing updates, but i need to likely take a more details pass, specifically points look janky with project tags since they are not in the same `li`.
Test Plan: Zoom into tags, see they all are same height and align.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15255
Summary: Mimics the Project Hovercards, more custom UI.
Test Plan: Hover over person with and without badges, hover over project.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15253
Summary:
Fixes T10330.
- Anywhere we support "matches regexp", also allow "does not match regexp". Although you can sometimes write a clever negative regexp, these rules are better expressed with "does not match <simple regexp>" anyway, and sometimes no regexp will work.
- Always allow "does not contain" when we support "contains".
- Fix some JS issues with certain rules affecting custom fields.
Test Plan:
- Wrote an "Affected files do not match regexp" rule that required every diff to touch "MANUALCHANGELOG.md".
- Tried to diff without the file; rejected.
- Tried to diff with the file; accepted.
- Wrote a bunch of "contains" and "does not contain" rules against text fields and custom fields, then edited tasks to trigger/observe them.
- Swapped the editor into custom text, user, remarkup, etc fields, no more JS errors.
{F1105172}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10330
Differential Revision: https://secure.phabricator.com/D15254
Summary: Also adds the commit to the header underneath the title. Ref T7628
Test Plan: Review a few Diffusion pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7628
Differential Revision: https://secure.phabricator.com/D15246
Summary: These are not needed I think? and handy for cut and paste. Fixes T7628
Test Plan: cut and paste easier from commit hash.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7628
Differential Revision: https://secure.phabricator.com/D15245
Summary:
These columns were conflating `projectPHID` (the defualt project to add to the task) with `boardPHID` (the board the column appears on).
Separate them to fix the beahvior.
Test Plan: Used "Add Task" from dropdown menu of a milestone column on a parent project's workboard.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15242
Summary: I missed this during cleanup.
Test Plan: Go to a project, then: Subprojects, Create Milestone, Cancel. No longer 404/fatals.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15241
Summary:
Fixes T8197. Currently, if you priority-sort a workboard and drag a card to the top or bottom, we change the priority even if we do not need to.
For example, if the lowest priority in a column is "Low", and you drag a "Wishlist" task underneath it, we incorrectly increase the priority of the task to "Low", when we do not actually need to touch it. This is bad/confusing.
A similar thing happens when dragging a "High" priority task to the top of a column where the highest priority is currently "Normal".
Test Plan:
- Create a column with a "Normal" task.
- Sort workboard by Priority.
- Drag a "High" task above it. After patch: task still "High".
- Drag a "Wishlist" task below it. After patch: task still "Wishlist".
Also dragged a ton of tasks into the middle of other tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8197
Differential Revision: https://secure.phabricator.com/D15240
Summary:
Fixes T6580. Now:
- Empty field means "unlimited".
- Zero means 0.
- Nonzero means that number.
(Although you can now have fractional points, I didn't change columns to allow fractional limits, because too bad.)
Test Plan: {F1103688}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6580
Differential Revision: https://secure.phabricator.com/D15239
Summary: Ref T4427.
Test Plan:
- Dragged a 17 XP task from "Hunting" to "Slain".
- Saw 17 XP move.
- Level up!
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D15237
Summary:
Ref T4900. Briefly:
- Much more layout and rendering is now done in Javascript.
- This should otherwise be identical to the behavior at HEAD, except that:
- editing a task and removing the current board from it no longer removes the task; and
- points still don't work.
However, this can now plausibly support realtime workboard updates and other complex state-based behaviors like points calculations in a future change.
Test Plan:
- Changed card covers.
- Moved cards.
- Sorted board by priority and natural.
- Added new cards.
- Edited cards in place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D15234
Summary:
Ref T4900. To eventually support realtime board updates, we need to be able to perform a board state update without the context of the action which caused it.
For example, if the server says "update card Y", we need to know what to do without being told "card Y was moved from column A to column B" explicitly. Currently, all the update code relies on knowing what happened and which nodes were affected.
This is only a small step forward, but starts making things a bit more independent and consistent.
Test Plan:
- Moved cards around.
- Changed card cover images.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D15228
Summary:
Ref T4900. Broadly, workboard state management is fairly ad-hoc now, which makes things like this (where some kind of edit affects global state) difficult:
- Updating points header to reflect a sum change after dragging a task.
- Changing progress bars after editing a task to change resolution or points value.
- Moving a card to the correct column after editing it and changing subprojects/iterations.
- Responding to real-time notifications about other users moving cards.
This begins rewriting the code in a way that can better accommodate these kinds of far-reaching state update.
This change just moves cover image stuff. I'll continue moving features one at a time until boards work better.
Test Plan: Updated some cover images.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D15224
Summary: Fixes T10316. Fixes T10311.
Test Plan:
- Create a project, create a milestone, go back to the parent, go to the workboard. Previously, fatal. Now, prompts you to create workboard.
- Create a project, create a milestone, create the parent workboard, put a task in the milestone's column, go to the milestone workboard. Previously, fatal. Now, prompts you to create workboard.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10311, T10316
Differential Revision: https://secure.phabricator.com/D15232
Summary:
Ref T4427.
- When points are configured, show them on the task detail page (just a simple property, at least for now).
- Typecast points better to avoid "joe changed points from 12 to 12." beacuse we compare the stored value (as a string) to the new value (as a double).
Test Plan:
- Saw points on detail view.
- Created task with points, then edited it without touching points. No more spurious "changed from 12 to 12" transaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D15223
Summary:
Ref T4427.
- New config option for labels, enabling, etc., but no UI/niceness yet.
- When enabled, add a field.
- Allow nonnegative values, including fractional values.
- EditEngine is nice and Conduit / actions basically just work with a tiny bit of extra support code.
Test Plan:
- Edited points via "Edit".
- Edited points via Conduit.
- Edited points via stacked actions.
- Tried to set "zebra" points.
- Tried to set -1 points.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D15220
Summary:
Ref T10288.
I couldn't figure out how to reasonably get the interior right borders to round like the mock, but I think this is otherwise mostly faithful. Feel free to fix stuff.
Test Plan: {F1100415}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10288
Differential Revision: https://secure.phabricator.com/D15219
Summary:
Currently never read or written.
Supports fractions.
There's no such thing as an unsigned double so this also supports negative values, technically, although I'll eventually prevent this in the UI.
Test Plan: `bin/storage upgrade`, then created and edited a task. Nothing was different.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15218
Summary:
Before edges, we stored some of this stuff directly on tasks.
- `attached` was migrated to edges in Jan 2013.
- `projectPHIDs` was never used, as far as I can tell?
- `ccPHIDs` was migrated away and dropped more than a year ago.
None of these columns are used in modern code (instead, modern code uses edges).
Test Plan: `grep`, browsed around, `bin/storage upgrade`, unit tests.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15216
Summary:
Fixes T7410.
- Adds a "Disable Workboard" action to the "Manage Backlog" menu.
- We'll probably move this somewhere else if/when that column gets too messy.
- Disabling a board hides it, prevents it from being recreated by non-editors, and hides the "Project (Backlog)" annotations.
- Resotring a board puts it back in pristine condition.
Test Plan:
- Disabled a board.
- Verified "(Backlog)" annotations vanished.
- Enabled a board.
Reviewers: chad
Reviewed By: chad
Subscribers: mbishopim3
Maniphest Tasks: T7410
Differential Revision: https://secure.phabricator.com/D15215
Summary: Fixes T10299.
Test Plan:
- Visited `/maniphest/?projects=x` locally, where `x` is some valid project slug.
- Before patch: Fatal on `requireViewer()` call.
- After patch: Works correctly, filling the correct project into parameters.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10299
Differential Revision: https://secure.phabricator.com/D15214
Summary:
Ref T10295
* Viewing Upcoming Events in the calendar would display 'Attending: ' even if there were not attendees. This caused confusion, such as 'Is it telling me I am "Attending?"'
* When a calendar event has no attendees, simply do not display the 'Attending: ' label
Test Plan:
* Add a new event with no one attending.
* Add a new event with one or more attendees.
* View the Upcoming Events query of the Calendar app.
* Notice how the one with no attendees does not show 'Attending: ' while the other with attendees will show the already existing 'Attending: jdoe, ssmith' label.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T10295
Differential Revision: https://secure.phabricator.com/D15207
Summary:
Ref T10010.
- Viewing an empty board with milestone columns did a meaningless edge query. Don't do that.
- When creating the first milestone of a parent, force the indexing engine to rematerialize it inline. This sets `hasMilestones` properly. Otherwise, the daemons may take some time to fix this in the indexer.
Test Plan:
- Viewed an empty board of a project with a milestone.
- Viewed a normal board.
- Created the first milestone of a project with a big queue of daemons, saw project state immediately fully reflect the project having milestones.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15206
Summary: Ref T10010. Allows you to click "Milestone 99" to jump directly to that project.
Test Plan:
- Clicked milestone header, went to milestone.
- Clicked normal column header, nothing happened. Wow!
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15204
Summary: This makes small cover images full-width instead of teeny tiny dots in the middle of an island of whitespace.
Test Plan: Uploaded a small cover image.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15203
Summary: This was slightly more complex than I believed, but not too terrible.
Test Plan:
{F1096126}
- Also used some normal file uploaders to make sure I didn't break that.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15202
Summary:
No way to set photos yet, but if you magic them in they work.
Primarily, this consolidates rendering logic so the move + edit + view controllers all run the same code to do tags / cover photos.
Test Plan: {F1095870}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15201
Summary:
Ref T10010. Milestones can't have subprojects, so this item isn't very useful.
I think there is also an argument for disabling "Members", but that panel is a little less useless and explains the membership rule, so I'm less certain about removing it. I do generally lean toward removing it at some point, though.
Test Plan:
- Viewed a milestone, no "Subprojects" menu item.
- Viewed a normal project, saw item.
- Edited both menus, saw consistent UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15200
Summary:
When filling in filler projects, only select active ones.
Also use a slightly more modern method signature.
Test Plan: Disabled a project, saw it vanish from the control.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15199
Summary:
Ref T10010. Instead of autojoining projects, provide "Initial Members: [___]" that the user can fill in.
This is only available in the web UI when creating a (non-milestone) project.
Test Plan:
- Created a new project with no members.
- Created a new project with some members.
- Created a new milestone (no control).
- Created a new project with myself as a member and an "Editable By: Project Members" policy, to verify this use case still works properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15195
Summary: Show on hovercards and the profile page itself.
Test Plan: Review a Milestone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15193
Summary: Minor, just fall back to the grey icon in all cases (too much color for me).
Test Plan: Review a Project and a Profile
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15190
Summary:
Ref T10289. This probably doesn't cover everything but should do a little bit better.
Although we should mabye just exlude milestones from this menu completely?
Test Plan: {F1093937}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10289
Differential Revision: https://secure.phabricator.com/D15191
Summary:
Fixes T10287. Ref T10286.
- Link stuff properly.
- Generally, show "Parent (Milestone)" instead of "Milestone".
- This probably doesn't get 100% of `getName()` -> `getDisplayName()` swaps, but we can get those as we catch them.
Test Plan: See T10286. Also clicked stuff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10286, T10287
Differential Revision: https://secure.phabricator.com/D15189
Summary:
Fixes T4136.
When listing projects in the "Visible To" selector control:
- Instead of showing every project you are a member of, show only a few.
- Add an option to choose something else which isn't in the menu.
- If you've used the control before, show the stuff you've selected in the recent past.
- If you haven't used the control before or haven't used it much, show the stuff you've picked and them some filler.
- Don't offer milestones.
- Also don't offer milestones in the custom policy UI.
Test Plan:
{F1091999}
{F1092000}
- Selected a project.
- Used "find" to select a different project.
- Saw reasonable defaults.
- Saw favorites stick.
- Tried to typeahead a milestone (nope).
- Used "Custom Policy", tried to typeahead a milestone (nope).
- Used "Custom Policy" in general.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4136
Differential Revision: https://secure.phabricator.com/D15184
Summary: Fixes T10281. Adds the closed icon (resolved, dupe, ect) as an attribute and makes the text grey again.
Test Plan:
View workboard with "All Tasks"
{F1092738}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10281
Differential Revision: https://secure.phabricator.com/D15187
Summary:
Ref T10010.
- Don't allow milestones to be reordered.
- Hide phantom subproject columns when reodrering.
- Don't allow subproject/milestone columns to be renamed.
- Force milestones to be ordered at the end, and in the correct order.
- Add some missing crumbs.
Test Plan: Reordered columns, renamed columns, made a new column, viewed column details.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15183
Summary: Fixes T10275. We'd fatal on `$flex` not being defined.
Test Plan: Uninstalled badges, viewed profile. Before: fatal; now: no badges element appears but profile renders properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10275
Differential Revision: https://secure.phabricator.com/D15182
Summary: Adds which Projects a user is a member of to their profile, with a link to more. Build fallback states for no badges or no projects.
Test Plan:
Review a user with projects, without projects, with badges, without badges.
{F1084127}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15150
Summary: Ref T10010. This gets rid of, e.g., the "Iteration I" tag in the column for that milestone, as it is redundant with the column itself.
Test Plan: {F1090427}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15181
Summary:
Ref T10010. These aren't perfect but I think (?) they aren't horribly broken.
- When a project is a parent project, destroy (as far as the user can tell) any custom columns.
- When a project has milestones, automatically generate columns on the project's workboard (if it has a workboard).
- When you move tasks between milestones, add the proper milestone tag.
- When you move tasks out of milestones back into the backlog, add the proper parent project tag.
- (Plenty of UI / design stuff to adjust.)
Test Plan:
- Dragged stuff between milestone columns.
- Used a normal workboard.
- Wasn't able to find any egregiously bad cases that did anything terrible.
{F1088224}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15171
Summary: Ref T10010. This isn't totally comprehensive, and a lot of behaviors aren't testable (e.g., all the Javascript stuff) but at least covers the basic create/move/reorder operations.
Test Plan: `arc unit`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15178
Summary: Ref T10010. This retires the old way of doing things inside ColumnPositionQuery. It is now obsolete and lives in BoardLayoutEngine instead.
Test Plan:
- Moved cards, created cards, swapped filters, orders, etc.
- Some degree of unit testing coming in the next diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15177
Summary: Ref T10010. This gets rid of the last dependency on the weird ColumnPositionQuery code.
Test Plan:
- Viewed workboards.
- Used batch editor.
- Created a new workboard.
- Dragged stuff around.
- Created new tasks into columns.
- Changed order from natural to priority, dragged things around.
- Switched filter to custom filter, "all tasks", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15176
Summary:
Ref T10010. See D15174. This gets rid of the "actually apply the change" callsite and moves it to layout engine.
Next up is to make the board view use the layout engine, then throw away all the whack code in ColumnPositionQuery, then move forward with D15171.
Test Plan:
- Dragged tasks within a column.
- Dragged tasks between columns.
- Dragged tasks to empty columns.
- Created a task in a column.
- Swapped board to priority sort, dragged a bunch of stuff all over.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15175
Summary:
Ref T10010. This is a precursor to D15171, which I'll eventually rebuild on top of these changes.
Currently, ColumnPositionQuery does a lot of "column layout" stuff that's very similar to the Milestone/Subproject stuff that needs to happen in D15171. The current approach there ended up splitting this layout stuff across two unrelated classes (ColumnPositionQuery + BoardViewController), neither of which is a particularly great place to do it -- the Query is too low-level, and the Controller is too high-level.
Instead, introduce a new "LayoutEngine" which does all this layout stuff. Swap two of the four places that we query this stuff over to the new engine:
- "Project (Column)" on tasks.
- Transaction generation when moving cards.
These sites aren't swapped by this diff, but will be by the next one:
- Actually applying transactions.
- Main layout for boards (this could swap easily now, but applying transactions currently relies on position writes having taken place, so it can't swap until the other one swaps).
Once everything is swapped over, I should be able to add the D15171 logic to LayoutEngine instead of BoardViewController and end up with a cleaner approach overall.
One particularly benefit is that //looking// at a board won't do a bunch of position writes anymore, which wasn't a big deal, but which I was a bit uneasy with.
Test Plan:
- Viewed tasks that are on boards, saw column annotations in project list.
- Moved cards between columns on a board.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15174
Summary: I don't think these ever had hovercards, but they should with subprojects/new design.
Test Plan: pointey pointey, got a card
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15180
Summary: Fixes T10273. The threshold is `null` if no chunk engines are available, but the code didn't handle this properly.
Test Plan: Disabled all chunk engines, reloaded, hit issue described in task. Applied patch, got clean file content.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10273
Differential Revision: https://secure.phabricator.com/D15179
Summary: No UI changes, just some search and replace for UI consistency.
Test Plan: Test person and object hovercards still work. UIExamples too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15172
Summary: Centers the page for consistency for the rest of Phame, puts blog list on right for better mobile support.
Test Plan: Review PhameHome at all breakpoints.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15170
Summary:
Ref T10010. When you try to add "Sprint 35" to a task, remove "Sprint 34", etc. Briefly:
- A task can't be in Sprint 3 and Sprint 4.
- A task can't be in "A" and "A > B" (but "A > B" and "A > C" are fine).
- When a user makes an edit which would violate one of these rules, preserve the last tag in each group of conflicts.
Test Plan:
- Added fairly comprehensive tests.
- Added a bunch of different tags to things, saw them properly exclude conflicting tags.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15167
Summary: Fixes T10259. There was no real reason to do this `ip2long()` stuff in the first place -- it's very slightly smaller, but won't work with ipv6 and the savings are miniscule.
Test Plan:
- Ran migration.
- Viewed logs in web UI.
- Pulled and pushed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10259
Differential Revision: https://secure.phabricator.com/D15165
Summary:
Ref T5240.
- Add proper class when dropping cards.
- Add proper class when creating new cards.
- Make X-drag explicit so that it works if there's only one column.
- Stop tootips when dragging, resume them after dropping.
- Move CSS rule for consistency.
- Allow user to hit "Escape" to cancel an in-progress drag.
Test Plan:
- Dropped cards.
- Created new cards.
- X-dragged on a workboard with one column and a dashboard.
- Dragged over a tooltip (no tip), dropped, moused over tooltip (tip).
- Hit escape during a drag.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T5240
Differential Revision: https://secure.phabricator.com/D15163
Summary:
Ref T5240. With the new approach, the draggable clones lose their containers, so they don't get affected by rules like `.container .item`.
Put classes on the cards/items and use `.board-item.item` and `.standard-item.item` to apply rules instead.
This didn't turn out //too// gross, and seems relatively OK / not obviously broken.
Test Plan:
- Dragged cards on a workboard.
- Dragged items in normal lists (tasks, pinned apps).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5240
Differential Revision: https://secure.phabricator.com/D15161
Summary: Reworks cards to add an assignee head and tooltip on workboards. This feels like a reasonable starting point, but they may move depending on feedback.
Test Plan:
View a lot of boards. Assign and unassign a task.
{F1085739}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Luke081515.2, Korvin
Differential Revision: https://secure.phabricator.com/D15158
Summary: I believe this got clobbered in rP8b6edaa4e238a809fe78e6d14ad0705545f8179f. This index doesn't seem to be present in the line dictionary and we're now relying on `$line_index` for the current position.
Test Plan:
before {F1085522}
after {F1085521}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D15156
Summary:
Mostly, this has just been sitting in my sandbox for a long time. I may also touch some charting stuff with subprojects/milestones, but don't have particular plans to do that.
D3 seems a bit more flexible, and it's easier to push more of the style logic into CSS so you can fix my design atrocities. gRaphael also hasn't been updated in ~3+ years.
Test Plan:
{F1085433}
{F1085434}
Reviewers: chad
Reviewed By: chad
Subscribers: cburroughs, yelirekim
Differential Revision: https://secure.phabricator.com/D15155
Summary: Fixes T10247. The flavor icons are unhelpful/confusing in these contexts; show a boringer icon instead.
Test Plan: Used tokenizer to select user with custom profile icon. Reloaded page. Saw boringer icon in both cases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10247
Differential Revision: https://secure.phabricator.com/D15154
Summary: Fixes T10242. Currently, we don't extract files, mentions, etc., properly from user profile blurbs.
Test Plan: Uploaded a file to my profile blurb, saw it attach properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10242
Differential Revision: https://secure.phabricator.com/D15153
Summary: Ref T10010. Restores subprojects and milestones to the UI with a more modern style and more warnings.
Test Plan:
{F1085207}
{F1085208}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15152
Summary:
Fixes T10250.
Rename the one I added to `hasAnyProperties()` for clarity.
Test Plan:
- Viewed a project profile with content.
- Viewed a project profile with no properties.
- Viewed a workboard with tasks that had a mixture of additional projects and no additional projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10250
Differential Revision: https://secure.phabricator.com/D15151
Summary: Designer, Musician, Spy, Robot
Test Plan: Click Choose Icon, see that I am a designer.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15147
Summary: Ref T10228. Commands like `git-http-backend` can emit errors with raw bytes in the output. Sanitize these if present so we can log them in JSON format.
Test Plan: Edited this into production. >_> sneaky sneaky <_<
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10228
Differential Revision: https://secure.phabricator.com/D15144
Summary: It feels wierd to edit a project or profile and not see the changes. For now add them back to the Manage page.
Test Plan: Edit a Profile, Edit a Project. See updates on Manage page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15140
Summary:
- Redirect to profileview when new image is uploaded.
- Add ProfileNav to EditPicture on Profile
- Add ProfileNav to EditProfile on Profile
Test Plan: Set new images on Profiles and Projects. See new redirect. See new navs.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15132
Summary: Fixes T10226. I just made a mistake here when rewriting this recently.
Test Plan: {F1079166}
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T10226
Differential Revision: https://secure.phabricator.com/D15131
Summary: Fixes T10176. The prefix is not useful in the JIRA context, and doubtfully useful in Asana.
Test Plan: Load, make comment on revision, see link in JIRA is pretty.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T10176
Differential Revision: https://secure.phabricator.com/D15119
Summary:
Ref T10228. This is currently quite limited:
- No UI.
- No SSH support.
My primary goal is to debug the issue in T10228. In the long run we can expand this to be a bit fancier.
Test Plan:
Made various valid and invalid clones, got sucess responses and not-so-successful responses, viewed the log table for general corresponding messages and broad sanity.
Ran GC via `bin/phd debug trigger`, no issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10228
Differential Revision: https://secure.phabricator.com/D15127
Summary:
A user in IRC seemed very confused by this, and worked extremely hard to shoot themsevles in the foot by manually writing locked configuration to the database.
Try to explain why configuration is locked better.
Test Plan:
Mostly reading.
{F1078905}
{F1078906}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15128
Summary: All our builtin images use #c4cde0 for the backdrop. This makes generation match the builtins.
Test Plan:
Build a new bug icon in Maniphest
{F1077934}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15126
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary: Reasonable first pass, removes the "light" header, due to pain of upkeep. Reinforces UI color into the Profile Nav (and later likely dropmenu hovers). Most of this is reasonably easy to maintain now, but I may do a more accurate color pass after I get some more time together with it. For now this feels pretty good if you're developing in a different color UI.
Test Plan:
Switch between all the colors, hover over all the states.
{F1076766}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15120
Summary:
- Better spacing for images
- Remove border
- White BG on Members page
Test Plan: Review Projects / Project Home / Project Members
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15118
Summary: Fixes T10213. I think the "Edit" item was originally conditional (or maybe I just forgot to add that part), but that got dropped when we swapped how it worked. This is all stable now anyway and can be available without needing prototypes enabled.
Test Plan: Edited a project menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10213
Differential Revision: https://secure.phabricator.com/D15117
Summary: Cleans up Workboards to match the mocks. No new functionality, just more consistent colors/spacing/common components.
Test Plan:
Visit a few workboards, drag and drop items. Mobile, Tablet, Desktop
{F1070733}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15115
Summary: Fixes T10217. This extra entry is just a mistake because of `rst` + `rest` both being valid suffixes. We don't need both entries.
Test Plan: Edited a paste, only saw one entry in dropdown for restructured text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10217
Differential Revision: https://secure.phabricator.com/D15114
Summary: Ref T10054. Prevent users from removing this item and locking themselves out of the system unless they can guess the URI.
Test Plan: Tried to disable "Manage", wasn't permitted to.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15113
Summary: See T10214 for context. These transaction types are obviously wrong as far as I can tell.
Test Plan:
Created a revision and didn't see an error in the daemon log.
```lang=php
<?php
require_once dirname(__FILE__).'/phabricator/scripts/__init_script__.php';
$yelirekim = (new PhabricatorPeopleQuery)
->setViewer(PhabricatorUser::getOmnipotentUser())
->withUsernames(['yelirekim'])
->executeOne();
$raw_diff = (new PhabricatorDifferenceEngine)
->generateRawDiffFromFileContent('oldfile', 'newfile');
$diff = (new ConduitCall('differential.createrawdiff', [
'diff' => $raw_diff,
]))
->setUser($yelirekim)
->execute();
$xactions = (new DifferentialDiffTransactionQuery)
->setViewer($yelirekim)
->withObjectPHIDs([$diff['phid']])
->execute();
foreach ($xactions as $xaction) {
echo $xaction->getPHID().':'.$xaction->getTitle().PHP_EOL;
}
```
for sanity
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: michaeljs1990, epriestley
Differential Revision: https://secure.phabricator.com/D15112
Summary: Updates People profiles to look more like Project profiles. This removes Conpherence and Flag links. Don't think you like Conpherence links much and for Flags maybe we can put them in the quick create menu?
Test Plan:
View profiles with and without Badges.
{F1069365}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15111
Summary: Minor point of polish, but feels really nice. Hover over photo and edit a link to change the picture.
Test Plan:
hover hover, clicky clicky
{F1069179}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15109
Summary:
Ref T10144. This isn't comprehensive, but we can give it a try and see how it feels?
- EditEngine forms now say "Tags" instead of "Projects".
- Modern SearchEngine forms now say "Tags" instead of "Projects".
- For clarity, replaced as much "in project" language as I could find with "tagged with project" language.
Test Plan: reading / grepping + used "not tagged with any project" token
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10144
Differential Revision: https://secure.phabricator.com/D15108
Summary: Fixes T6501. This adds more API information to the newish `project.search` API method.
Test Plan: Called `project.search`, used attachments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6501
Differential Revision: https://secure.phabricator.com/D15105
Summary: This improves consistency (by making this UI more similar to the projects UI) and gives us more flexibility the next time we update user profiles.
Test Plan:
{F1068889}
Took all the actions (probably?) to check that all the redirects were updated.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15104
Summary: Ref T10054. This primarily improves aesthetics and consistency for member/wathcher lists in projects.
Test Plan:
{F1068873}
{F1068874}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15103
Summary: Moves some profile css into PHUI, cleans up mobile view and desktop spacing.
Test Plan: Test Project at desktop and mobile breakpoints.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15106
Summary: Ref T10054.
Test Plan:
{F1068420}
Also looked at a project which did have stuff to make sure the stuff still worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15102
Summary: Fixes T10212. This method was removed in D14990, but I missed a callsite.
Test Plan: Disabling blame now works nicely.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10212
Differential Revision: https://secure.phabricator.com/D15100
Summary: I moved history to manage and missed this callsite.
Test Plan: Use present icon/color quick select.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15099
Summary:
First pass at a new Project Home page. This is starting to sprawl, so punting this up now before it gets too large.
- Project homes now have "large header"
- Custom Fields / Descriptions are in the main column
- Feed is simpler visually
- new "Background" option for PHUIObjectBoxView
- move header buttons globally to "Grey" instead of "Simple"
- New color and hover states for "Grey"
- Transitions on Buttons haha
- Edit Icon on Nav is now under "Manage" panel
- New "Manage" Panel
TODO:
- More testing of bad cases of Custom Fields
- Members Page in flux, needs design
- Um still not sure how to make Custom Field not show UI
Test Plan:
Lots of random Project page visits. Save project, watch project, edit project, etc.
{F1068191}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15097
Summary:
Fixes T10211. This transaction can either be setting or removing the "destroyed" flag, but we show "destroyed" in both cases.
Instead, if the transaction is clearing the flag, render "restored".
Test Plan: {F1068142}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10211
Differential Revision: https://secure.phabricator.com/D15096
Summary: Ref T10054. Since we no longer have the "workboard default if it exists" rule, provide a quick way to make it the default.
Test Plan: Created a new workboard with the box checked, saw menu change appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15092
Summary: racooons → racoons
Test Plan: Read again the sentence.
Reviewers: #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D15094
Summary:
Ref T10054. Ref T6961.
- Existing projects with workboards had "Workboard" as the default menu item. Retain this behavior.
- Populate the recently-added `hasWorkboard` flag so we can do a couple of things a little faster (see T6961).
Test Plan:
- Ran migration.
- Verified a bunch of projects looked sensible/correct after the migration.
- Created a workboard, verified `hasWorkboard` got set properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6961, T10054
Differential Revision: https://secure.phabricator.com/D15093
Summary:
Ref T10054. Uncreated workboards feel a little awkward right now because you lose the menu. Instead, keep the menu.
I also plan to:
- add a "[X] Make the workboard the default view for this project." checkbox; and
- resolve T6961.
...which will touch this workflow, so modernize/straighten it out.
Test Plan:
Viewed workboard, no access state, empty state. Created empty board, imported board.
{F1066973}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6961, T10054
Differential Revision: https://secure.phabricator.com/D15091
Summary: Ref T10054. This is all pretty straightforward. Also include some project-specific examples in the project documentation.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15090
Summary:
Ref T10054.
- Just let users delete non-builtin items.
- Let users choose a default item explicitly.
- Do a better job of cleaning up items which no longer exist or belong to uninstalled applications.
(NOTE) This has one user-facing change: workboards are no longer the default on projects with workboards. I think this is probably OK since we're giving users a ton of new toys at the same time, but I'll write some docs at least.
Test Plan:
- Deleted custom items.
- Disabled/enabled builtin items.
- Made various things defaults.
- Uninstalled Maniphest, saw Workboards tab disappear entirely.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15089
Summary: Fixes T5819. Adds configuration for setting color labels on projects and changing the default. Options are locked to what we make available.
Test Plan: {F1066823}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5819
Differential Revision: https://secure.phabricator.com/D15088
Summary: `alterClass()` is strict about true/false but we set 0/1 elsewhere.
Test Plan: Collapsed/expanded menu, reloaded expanded menu, clicked collapse, got immediate collapse.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15087
Summary:
Fixes T10196. This is a weird interaction and this might not be the best long-term fix, but just get it working OK for now.
General problem is that Quicksand doesn't currently use GET for requests. This is a very unusual case where the method is relevant. In the future, I might change Quicksand to use GET.
Test Plan: Clicked "Open Tasks" with Quicksand active, got a results list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10196
Differential Revision: https://secure.phabricator.com/D15082
Summary: Mostly a visual spacing pass, also adds in circle icons for edit, collapse. For now removing the fixed position on the icons for simplicity while the basics are being polished.
Test Plan: Projects, Profiles, wide and narrow.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15081
Summary:
Ref T10195. Distinguish between "database does not exist" and "database exists, you just don't have permission to access it".
We can't easily get this information out of INFORMATION_SCHEMA but can just `SHOW TABLES IN ...` every database that looks like it's missing and then look at the error code.
Test Plan:
- Created a user `limited` with limited access.
- Ran `bin/storage adjust`.
- Got hopefully more helpful messages about access problems, instead of "Missing" errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10195
Differential Revision: https://secure.phabricator.com/D15079
Summary: Fixes T10192. This key improves some common queries and is not currently present.
Test Plan: See discussion in T10192. Verified current query plan of real queries is garbage and improved by adding this key.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10192
Differential Revision: https://secure.phabricator.com/D15075
Summary:
Fixes T10186. After D14970, `diffusion.filecontentquery` puts the content in a file and returns the file PHID.
However, it does this in a way that doesn't go through the chunking engine, so it will fail for files larger than the chunk threshold (generally, 8MB).
Instead, stream the file from the underlying command directly into chunked storage.
Test Plan:
- Made a commit including a really big file: 4dcd4c492b
- Used `diffusion.filecontentquery` to load file content.
- Parsed/imported commit locally.
- Used `diffusion.filecontentquery` to load content for smaller files (README, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10186
Differential Revision: https://secure.phabricator.com/D15072
Summary: We plan to use these more in future mocks. Adds base colors and re-uses in Phame.
Test Plan: Phame, mobile and desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15069
Summary: Ref T10054. This is mostly for completness so I can reference it when closing all the related tasks.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15066
Summary:
Ref T10054. Ref T6113. Users can currently subscribe to projects, which causes them to receive:
# mail about project membership changes, description changes, etc; and
# mail to the project, e.g. when the project is added as a subscriber on a task, or a reviewer on a revision.
Almost no one cares about (1), and after D15061 you can use Herald to get this stuff if you really want it. (It will get progressively more annoying in the future with external membership sources causing automated project membership updates.)
A lot of users are confused about (2) and how it relates to membership, watching, etc, and most users who want (2) don't want (1).
Instead, add an explicit option for this and explain what it does.
This is fairly verbose but I've hidden it on the member/watch screen, which is now the "explain how projects work" screen, I guess.
Test Plan:
{F1064929}
{F1064930}
{F1064931}
- Disabled/enabled mail for a project.
- Sent mail to a project with mail disabled, verified I didn't get a copy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6113, T10054
Differential Revision: https://secure.phabricator.com/D15065
Summary:
Ref T6183. Ref T10054. Historically, only members could watch projects because there were some weird special cases with policies. These policy issues have been resolved and Herald is generally powerful enough to do equivalent watches on most objects anyway.
Also puts a "Watch Project" button on the feed panel to make the behavior and meaning more obvious.
Test Plan:
- Watched a project I was not a member of.
- Clicked the feed watch/unwatch button.
{F1064909}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6183, T10054
Differential Revision: https://secure.phabricator.com/D15063
Summary: Ref T10054. There is no technical or product reason not to support this, and it is largely analogous to removing subscribers.
Test Plan:
- Removed watchers.
- Removed members.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15062
Summary:
Ref T10054. Ref T6113. I'm going to remove subscribers from projects to fix the confusion between "watch" and "subscribe".
Users who have unusual use cases where they legitimately want to know when a project's description is updated or members change can use Herald to follow it.
This is also useful in general and improves consistency, although I don't have too many use cases for it.
Test Plan: Wrote a Herald rule, edited a project, saw the rule fire and send me email about the change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6113, T10054
Differential Revision: https://secure.phabricator.com/D15061
Summary:
Ref T10054. This shuffles some stuff around to move us closer to mocks in M1450 in terms of what information is on which pages.
Home now has feed, members, watchers, link to "edit project / project edit history".
History now has edit history, edit details, edit picture, archive/unarchive.
Test Plan:
New home page:
{F1064889}
New edit/history page:
{F1064890}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15060
Summary:
Ref T10054. This tries to make the members page a bit more consistent and provide hints to users about subproject/milestone membership rules. In particular:
- You now join, leave, watch, unwatch, add and remove members, and lock and unlock membership from the members screen.
- We now explain the membership rule for the project on this screen. There are currently four rules:
- Normal Project: Join/leave normally.
- Parent Project: Uses subprojects to determine members.
- Milestone: Uses parent project to determine members.
- Locked: Membership is locked.
- (Future) Imported from LDAP/other external sources: Membership is determined by something else.
Test Plan: {F1064878}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15059
Summary:
Ref T10054. I think this gets everything except:
- circles on icons;
- I spent ~15 minutes poking at animations but wasn't able to get anything that looked reasonable whatsoever.
Test Plan:
- Collapsed menus.
- Expanded menus.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15056
Summary: Ref T10010. Fixes T10107. When we publish a transaction about a project, we perform visibility checks for many different users. We need to know all of the ancestors' members to perform these checks.
Test Plan:
- Before patch: when updating a subproject, daemons fatal trying to publish things because they can not test visibility of parent projects.
- After patch: daemons successfully publish subproject updates.
- Also added a unit test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010, T10107
Differential Revision: https://secure.phabricator.com/D15054
Summary: Ref T10054. The daemons look for this but currently can't find it.
Test Plan: Ran daemons, clean exit on profile menu edits instead of permanent failure.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15053
Summary: These are old project image choices, remove and only go with FontAwesome related images.
Test Plan: Project -> Edit Picture -> Save
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15051
Summary: Removes header gradient images for flat, CSS controlled colors. I didn't convert the "pony" colors over, going with few options for easier theme-ability.
Test Plan:
Test each color choice.
{F1063828}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15052
Summary:
Fixes T6856. Fixes T10164.
- Make the profile image composer code use the underlying icon name instead of the top-level icon key, so it works instead of 404'ing.
- Change the button to show a preview of the profile icon instead of the text "Use Icon and Color".
- When creating a new non-milestone project, automatically set the profile image to the icon + color image.
Test Plan:
- Created several new projects, saw appropriate default icons.
- Edited projects, saw icon previews.
- Clicked icon buttons to set icons.
- Poked around other applications which use builtins (Pholio, user profiles) to look for anything I broke, but everything seemed fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6856, T10164
Differential Revision: https://secure.phabricator.com/D15050
Summary: Ref T4245. Fixes T10172. These regular expressions were simply incorrect: they intend `<start> (form one | form two) <end>` but were written as `(<start> form one) | (form two <end>)` which allowed stuff like "R2/R13" to be interpreted as a monogram because it matches `(<start> form one)`.
Test Plan: Parsed commit `ba46ffa6169c` from RTEMS repository, see T10172. Before patch, got an identical trace; after patch, clean import.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T4245, T10172
Differential Revision: https://secure.phabricator.com/D15049
Summary:
Ref T10168. When we try to remove an additional hashtag, we remove the normalized version.
Instead, remove both the literal and normalized versions. This allows us to remove old/invalid slugs.
Test Plan: Removed garbage slugs like `[,*,]`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10168
Differential Revision: https://secure.phabricator.com/D15048
Summary:
Ref T10168.
Around October 12, T9551 made project hashtags stricter and prevented them from containing characters like comma (`,`).
Around December 27, D14888 changed how hashtags queries work so that the query does normalization instead of requiring the caller to normalize.
After the Dec 27 change, projects from before Oct 12 with now-invalid hashtags will no longer load when queried directly by hashtag, because the page queries for `old,[silly]hash,,tag` or whatever, it gets normalized into `old_silly_hash_tag`, and then there are no hits.
Instead, at least for now, query by both the exact raw text and the normalized hashtag. This should keep older stuff working until we can give users more support for migrating forward.
Test Plan:
- Forced a project to have a bogus hahstag.
- Before patch: clicking its tag 404'd.
- After patch: clicking its tag now works.
- Visited a project by alternate hashtag.
- Visited a project by denormalized hashtag and alternate hashtag (e.g., capital letters instead of lowercase letters), saw it redirect/normalize properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10168
Differential Revision: https://secure.phabricator.com/D15047
Summary: Icon in a circle. Base class, not much in the way of color choices.
Test Plan:
UIExamples, Chrome.
{F1062027}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15034
Summary: Ref T10168. When we render this control, we currently don't put commas into the value correctly if there are multiple alternative hashtags.
Test Plan: Edited a project with multiple alternate hashtags. Before change: they all got smushed together. After change: properly comma-separated.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10168
Differential Revision: https://secure.phabricator.com/D15045
Summary: Fixes T10169. Diffs with no build targets were incorrectly showing as though they had no test coverage, when we actually want to show them having no coverage information available.
Test Plan: Viewed an older revision, saw a column of "Not Executable" before change, now see no column.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10169
Differential Revision: https://secure.phabricator.com/D15044
Summary: Fixes T10167. We were dropping infrastructure joins.
Test Plan: Queried for questions by project.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10167
Differential Revision: https://secure.phabricator.com/D15043
Summary: Motivate your employees with inspirational quotes. A new quote every day!
Test Plan: So inspirational.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15026
Summary:
Ref T10054. Primary goal is to be able to remove IconNav from the codebase.
I've made these non-editable so users can't customize them yet. We //might// want administrators to customize these globally instead? In any case, we avoid a bunch of product questions by just locking these down for now.
Test Plan: {F1061348}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15020
Summary: Ref T10054. Take specialization off the objects and put it on Engine subclasses instead. One reason for this is that certain objects (like users) might have multiple different sets of panels in the future (e.g., their user profile and their home page).
Test Plan:
- No user-visible changes.
- PanelEngine no longer has any hardcoded "project" stuff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15018
Summary:
Ref T10054. This makes the profile menu full-height. It uses two pieces of dark magic:
- `calc()`, which allows you to do math in CSS.
- The `vh` unit, which is CSS for "viewport height".
Apparently this kind of stuff just works now? CSS got good at some point?
Test Plan:
- Page looks correct in Safari, Chrome, Firefox.
- Checked `caniuse.com` for `vh` and `calc()`, saw they're supported?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15017
Summary:
Ref T10054. I haven't done any of the big-picture layout stuff yet, but this should get look-and-feel somewhere in the ballpark of reasonablness, I think.
Major missing stuff:
- No "collapse" state or action yet.
- Menu is not full-height (requires changes to the rendering pipeline).
Test Plan: {F1060941}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15016
Summary: Ref T3725. This probably has 900,000 bugs. This will need updates for subprojects/milestones.
Test Plan:
- Tested very gently in Safari, Firefox and Chrome.
- Reasonable inputs appear to work.
- Clicking, escape, tab, return, arrow keys work OK?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3725
Differential Revision: https://secure.phabricator.com/D15029
Summary: Ref T10054. Just simplifying this a bit before I start laying in the new profile menus.
Test Plan:
- Viewed Diviner on desktop and checked the mobile menu.
- Viewed Files on desktop and checked the mobile menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15015
Summary:
Ref T10054.
I made this a dropdown (currently: "Visible" or "Disabled") since I imagine we //miiiight// want to add a "Hidden, but click 'More' to reveal" state or do other special stuff in this vein. Not 100% sold on that but seemed within the realm of plausibility.
Test Plan: {F1060759}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15012
Summary: Ref T10054. Allows users to drag menu items to reorder them.
Test Plan: Reordered a project menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15011
Summary:
Ref T10054. This does a big chunk of the legwork to let users reconfigure profile menus (currently, just project menus).
This includes:
- Editing builtin items (e.g., you can rename the default items).
- Creating new items (for now, only links are available).
This does not yet include:
- Hiding items.
- Reordering items.
- Lots of fancy types of items (dashboards, etc).
- Any UI changes.
- Documentation (does feature: TODO link for documentation).
Test Plan:
{F1060695}
{F1060696}
{F1060697}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15010
Summary:
hack hack hack
(`class_exists()` no longer throws in a libphutil environment.)
Test Plan: derpaderp
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15013
Summary: Fixes T10139. This clarification seems reasonable to me.
Test Plan:
- Viewed a revision with context.
- Clicked "Show All Context".
- Saw "All Context Shown".
{F1060624}
{F1060625}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10139
Differential Revision: https://secure.phabricator.com/D15009
Summary: Ref T10054. This mostly makes sure that mobile gets to have the same profile menu that Desktop does.
Test Plan:
- Viewed menus on mobile, saw all profile menu actions available.
- Viewed/used changed pages (column detail, edit column, edit picture).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15008
Summary: Ref T10054. This has no product impact, but prepares us for customizable side nav on "profiles" (today, projects; probably users some day; and maybe other stuff down the road).
Test Plan: Clicked all links on a profile, everything was exactly the same as before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15007
Summary:
Ref T4705 (there are also some other adjacent related tasks dealing with URIs).
Currently, we issue a "get repositories matching URIs: ..." query by loading every possible repository and then checking their URIs in PHP.
Instead, put URIs in a separate table. I plan for each repository to potentially have multiple URIs soon, so this prepares for that.
Test Plan:
- Ran migrations.
- Looked at index table, made sure it appeared sensible.
- Ran some queries by `uri` to find repositories, found the repositories I expected.
- Updated the remote URI of a repository, saw queries / index update appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4705
Differential Revision: https://secure.phabricator.com/D15005
Summary: Fixes T10096. The `**` was being parsed as part of the hashtag, so `**#asdf**` interpreted `#asdf**` as a hashtag.
Test Plan: Unit test; bolded stuff with hashy contents.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10096
Differential Revision: https://secure.phabricator.com/D15006
Summary:
Fixes T10135. When the viewer is a member of no projects, specify the constraint type as a new "EMPTY" type.
When a query has an "EMPTY" constraint, fail fast with no results.
Test Plan:
- Viewed a viewerprojects() query result set as a user in no projects.
- Before patch: got a lot of hits. After patch: no hits.
- Viewed a normal result set, no changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10135
Differential Revision: https://secure.phabricator.com/D15003
Summary: A lot of confusing stuff happens when you create a milestone or subproject which isn't explained clearly in the UI. This is causing more harm than good on the balance since we're still figuring out how to move forward here. Just turn it off for now until we're closer to pushing it forward.
Test Plan: Viewed a project, no more UI for subprojects or milestones.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14999
Summary:
See IRC. We're supposed to repair configuration, but if custom validators throw a generic `Exception` or use `PhutilTypeSpec` to do a check, we may explode way harder than we intend to.
Instead, soften these exceptions into validation exceptions so we repair configuration, raise a setup issue, and continue.
Test Plan: {F1059609}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14998
Summary:
Ref T10121. This doesn't work at all at HEAD, and even when it did it was mostly just confusing to installs with unexpected setups where Phabricator is receiving mail at `@mycompany.com` and this is colliding with real addresses.
It might make sense to restore it some day after the next Conphernece update, but just strip it out for now. Since it doesn't work anyway, I'm pretty confident no one is using it.
Test Plan:
- Before patch: send mail to `dog@local.phacility.com`, got a policy error from Conpherece.
- After patch: sent mail to `dog@local.phacility.com`, got a correct "no routable recipients" error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10121
Differential Revision: https://secure.phabricator.com/D14997
Summary:
Ref T10121. If a user sends mail with duplicate headers, like:
```
X-Duplicate: A
X-Duplicate: B
```
...and we process it with `mail_handler.php`, we may end up with `array('A', 'B')` as the header value. When we try to write this back into an error response mail, it fails.
Test Plan:
- Generated a message with duplicate headers.
- Piped it into `mail_handler.php` with `--process-duplicates` and `--trace` to get a look at it.
- Faked an exception.
- Before patch: bad error email.
- After patch: clean error email showing multiple header values.
```
$ ./bin/mail show-outbound --id 12386
PROPERTIES
ID: 12386
Status: queued
Related PHID:
Message:
PARAMETERS
sensitive: 1
is-error: 1
force: 1
subject: Error Processing Mail (Exception)
raw-to: ["epriestley@phacility.com"]
HEADERS
X-Phabricator-Sent-This-Message: Yes
X-Mail-Transport-Agent: MetaMTA
X-Auto-Response-Suppress: All
TEXT BODY
Your email to Phabricator was not processed, because an error occurred while
trying to handle it:
Exception: TEST
-- Original Message Body -----------------------------------------------------
testy testy
-- Original Message Headers --------------------------------------------------
from: Evan Priestley <epriestley@phacility.com>
content-type: text/plain; charset=us-ascii
content-transfer-encoding: 7bit
x-smtp-server: smtp.gmail.com:epriestley@phacility.com
subject: test outbound mail
message-id: 7isvptmllqvdvtdxthvdwzg3woj5au7csyuh3hopypjv6y6hqb32qm4bcrd4jtid
x-universally-unique-identifier: 4E489E20-F674-49B2-94BA-0DE44F504EAA
date: Mon, 11 Jan 2016 09:50:12 -0800
date: Mon, 11 Jan 2016 09:50:13 -0800
date: Mon, 11 Jan 2016 09:50:14 -0800
date: Mon, 11 Jan 2016 09:50:15 -0800
to: epriestley@yghe.net
mime-version: 1.0 (Mac OS X Mail 8.2 \(2104\))
HTML BODY
(This message has no HTML body.)
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10121
Differential Revision: https://secure.phabricator.com/D14996
Summary: Fixes T8826. Git tracks an "author date", which may be different from the "committed date". We don't currently extract/show this; do so.
Test Plan: {F1059235}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8826
Differential Revision: https://secure.phabricator.com/D14995
Summary:
Fixes T10117.
- I accidentally broke setting `null` to unassign tasks at some point when I added richer validation.
- Raise a better error if the user passes junk.
Test Plan:
- Unassigned a task via API and web UI.
- Reassigned a task via API and web UI.
- Tried to do an invalid assign via API, got a sensible error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10117
Differential Revision: https://secure.phabricator.com/D14992
Summary: Ref T4245. Full commit display names (like `rPaaaa`) are going to be obnoxious soon in some cases (e.g., `rPaaaa` becomes `R123:aaaa`, which is much uglier) so reduce how often we show the repository in cases where it isn't really necessary to include it.
Test Plan:
- Saw no more `rX` on repository list view for Git/Mercurial (still present for Subversion).
- Saw no more `rX` on various repository detail views, except when referencing other commits (e.g., mentions).
- Grepped for removed `getShortName()`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14990
Summary:
Ref T4245.
- Rename "Clone/Checkout As" to "Short Name" in the UI.
- Allow any repository to have a short name, not just hosted repositories.
Test Plan:
- Ran migration.
- Reviewed old transactions, saw they looked good.
- Edited an existing repository's short name.
- Gave an imported repository a new short name.
- Removed a repository's short name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14989
Summary:
Ref T4245. This allows `bin/repository update bread` to work, in addition to `rBREAD`, `R123`, `123`, `BREAD`, etc., if a repository has a short name set.
This primarily affects CLI commands (like `bin/repository`) and Conduit API calls. It has no normal user-facing impact.
Test Plan: Ran `bin/repository update bread` and such.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14988
Summary:
Via HackerOne. The use of `$key` here should be `$extended_key`.
Exploiting this requires a very unusual group of objects to be subjected to extended policy checks. I believe there is no way to actually get anything bad through the policy filter today, but this could have been an issue in the future.
Test Plan:
- Added a unit test which snuck something through the policy filter.
- Fixed use of `$extended_key`.
- Test now passes.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14993
Summary:
Fixes T7938.
- Primarily, users can currently shoot themselves in the foot by putting `../../etc/passwd` and other similar nonsense in these fields (this is not dangerous, but also does not work). Require sensible names.
- Enforce uniqueness so these names can be used in URIs and as identifiers in the future.
- (This doesn't start actually using them for anything fancy yet.)
Test Plan:
- Gave several repositories clone names: a valid name, two duplicate names, an invalid, name, some with no names.
- Ran migrations.
- Got clean conversion for valid names, appropriate errors for invalid/duplicate names.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7938
Differential Revision: https://secure.phabricator.com/D14986
Summary:
If you try to pull the hovercard of something you can no longer see (maybe you loaded the page, then the policy changed) there won't be a value in the array here.
(The rest of the code anticipates this possibility.)
Test Plan: Hovered some stuff.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14987
Summary: See Q266.
Test Plan: Created a bulk job, clicked "Details" instead of "Confirm", clicked "Continue" to get back to confirmation dialog.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14985
Summary:
Ref T5155. Swaps Phabricator over to the new first-party S3 client using the v4 authentication API so it works in all regions.
The API requires an explicit region, so the new `amazon-s3.region` is now required. I'll write guidance about this.
Test Plan:
- Uploaded files to S3.
- Migrated ~1GB of files to S3.
- Loaded a bunch of files off S3.
- Browsed around the S3 bucket.
- Deleted a file, verified the data on S3 was destroyed.
- Hit new setup warning.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5155
Differential Revision: https://secure.phabricator.com/D14982
Summary: Ref T10110. If an image had `width` or `height`, we would accidentally not give it an `$image_class`.
Test Plan:
{F1057988}
{F1057989}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10110
Differential Revision: https://secure.phabricator.com/D14983
Summary: Ref T9828. Mostly just does a minor modernization pass, but also doesn't migrate chunked files since it's not meaningful (they don't have data, directly).
Test Plan: Ran `bin/files migrate` with various flags. Migrated S3 -> Blob and Blob -> S3.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9828
Differential Revision: https://secure.phabricator.com/D14981
Summary:
Ref T8612. If a change affects more than 10K paths + hunks, tell the user it's too big and don't bother trying to write it. We're mostly bounded by INSERTs here.
Also, fix an issue with file upload errors. The keys are real PHP constants, but were accidentally converted to strings in D12797, causing every error to show as "unknown error".
Test Plan: {F1057509}
Reviewers: joshuaspence
Reviewed By: joshuaspence
Maniphest Tasks: T8612
Differential Revision: https://secure.phabricator.com/D14977
Summary: I missed this in the recent icon customziation thing.
Test Plan: Typehaead'ed some projects, saw icons properly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14975
Summary:
Ref T10010. Ref T5819. General alignment of the stars:
- There were some hacks in Conduit around stripping `fa-...` off icons when reading and writing that I wanted to get rid of.
- We probably have room for a subtitle in the new heavy nav, and using the icon name is a good starting point (and maybe good enough on its own?)
- The project list was real bad looking with redundant tag/names, now it is very slightly less bad looking with non-redundant types?
- Some installs will want to call Milestones something else, and this gets us a big part of the way there.
- This may slightly help to reinforce "tag" vs "policy" vs "group" stuff?
---
I'm letting installs have enough rope to shoot themselves in the foot (e.g., define 100 icons). It isn't the end of the world if they reuse icons, and is clearly their fault.
I think the cases where 100 icons will break down are:
- Icon selector dialog may get very unwieldy.
- Query UI will be pretty iffy/huge with 100 icons.
We could improve these fairly easily if an install comes up with a reasonable use case for having 100 icons.
---
The UI on the icon itself in the list views is a little iffy -- mostly, it's too saturated/bold.
I'd ideally like to try either:
- rendering a "shade" version (i.e. lighter, less-saturated color); or
- rendering a "shade" tag with just the icon in it.
However, there didn't seem to be a way to do the first one right now (`fa-example sh-blue` doesn't work) and the second one had weird margins/padding, so I left it like this for now. I figure we can clean it up once we build the thick nav, since that will probably also want an identical element.
(I don't want to render a full tag with the icon + name since I think that's confusing -- it looks like a project/object tag, but is not.)
Test Plan:
{F1049905}
{F1049906}
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4, Luke081515.2
Maniphest Tasks: T5819, T10010
Differential Revision: https://secure.phabricator.com/D14918
Summary:
Fixes T9323. Two minor fixes:
- On the first commit, don't render a downward line.
- Clean up a 1px spacing issue that had cropped up a while ago when we added icons or something, I think.
Test Plan:
Before:
{F1057248}
After:
{F1057249}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9323
Differential Revision: https://secure.phabricator.com/D14974
Summary:
Fixes T9319. Proxied requests (e.g., in the cluster) for binary files (like images) currently fail because we can not return binary data over Conduit in JSON.
Although Conduit will eventually support binary-safe encodings, a cleaner approach to this is just to return a `filePHID` instead of the raw content. This is generally faster and more flexible, and gives us more opportunities to add caching later.
After making the call, the client pulls the file data separately.
We also no longer need to return a complex data structure because we don't do blame over this call any longer.
Test Plan:
- Viewed images in Diffusion.
- Viewed READMEs in Diffusion.
- Used `bin/differential attach-commit rX Dy` to hit attach pathway.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14970
Summary: Ref T9319. Ref T2783. This won't currently work in a future environment where daemons and repositories are not on the same host. Send it over Conduit instead.
Test Plan: Used `bin/differential attach-commit rX Dy` to force attachment, saw valid content pull over Conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2783, T9319
Differential Revision: https://secure.phabricator.com/D14969
Summary:
Ref T9319. See D14967. As before, this is making a deeply-buried, complex operation easier to test by providing a CLI command.
This adds `bin/differential attach-commit rXnnnn Dnnnn` to pretend that `rXnnnn` was just committed and matched `Dnnnn`.
Test Plan:
- Ran `bin/differential attach-commit X Y` for several different values, saw updates in the UI.
- Faked the message parser to make sure stuff still worked there.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14968
Summary: Fixes T9701. I don't want to try to autofix this because destroying the directory could destroy important files, but we can improve the error message.
Test Plan: Faked a failure, ran `repository update X`, got a more tailored error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9701
Differential Revision: https://secure.phabricator.com/D14971
Summary:
The old treatment was fairly technical. Give this UI a more human-friendly flow:
- Use language "remove" instead of "disable". We keep the record that the card existed around for auditing/historical purposes, but it is no longer a valid payment method going forward and can not be undone. I think this aligns with user expectation and actual behavior better than "disable".
- Only show active methods on the profile screen.
Test Plan: {F1057153}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14973
Summary:
Ref T9319. When we discover a commit, we sometimes update the corresponding revision with a "this is the actual committed change" diff and send out a link to the changes between review and commit.
This is currently very difficult to test, because it only happens the first time and you have to either go set up a bunch of objects or add a bunch of special casing to the parser to hit the workflow.
I'm making some changes to how it pulls file content. To make those changes easier to test, first start extracting this stuff so the code can be run with `bin/differential extract ...` instead of needing to do a bunch of more complicated setup steps.
Test Plan:
- Ran `bin/differential extract ...` to extract diffs from commits.
- Forced my way through the daemon workflow by faking out a bunch of flags, got a clean extract + attach + update. After this patch, this should rarely be necessary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14967
Summary: I was looking at some random un-revisioney repository for most of my testing and missed these.
Test Plan: Viewed blame of a file with some revisions.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14965
Summary: Fixes T2450. If we spend more than 15 seconds in blame, just cut it off.
Test Plan:
- Changed timeout to 0.01 seconds.
- Did blame on a non-highlighted file, got no blame, saw warning.
- Did blame on a highlighted file, got no blame.
- Note: you don't get a warning here because of Ajax stuff. It'd be kind of tricky to add and doesn't seem like a big deal so I'm planning to leave it as-is for now.
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4, chasemp
Maniphest Tasks: T2450
Differential Revision: https://secure.phabricator.com/D14964
Summary:
Ref T2450. This reorganizes code to improve performance.
Mostly, there are a lot of things which are unique per commit (author name, links, short name, etc), but we were rendering them for every line.
This often meant we'd render the same author's name thousands of times. This is slower than rendering it only once.
In 99% of interfaces this doesn't matter, but blame is weird and it's significant on big files.
Test Plan:
Locally, `__phutil_library_map__.php` now has costs of roughly:
- 550ms for main content (from 650ms before the patch).
- 1,500ms for blame content (frrom 1,800ms before the patch).
So this isn't huge, is a decent ~20%-ish performance gain for shuffling some stuff around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2450
Differential Revision: https://secure.phabricator.com/D14963
Summary:
Ref T2450. File blame tends to have the same commit a lot of times, and we don't do lookups like this efficiently right now.
In particular, for a file like `__phutil_library_map__.php`, we would issue a query with ~9,000 clauses like this:
```
(repositoryID = 1 AND commitIdentifier LIKE "XYZ%")
```
...but only a few hundred of those identifiers were unique. Instead, issue only one clause per unique identifier.
MySQL also seems to do a little better on "commitIdentifier = X" if we have the full hash, so special case that slightly.
Test Plan:
- Issuing a query for only unique identifiers dropped the cost from 400ms to 100ms locally.
- Swapping to `=` if we have the full hash dropped the cost from 100ms to 75ms locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2450
Differential Revision: https://secure.phabricator.com/D14962
Summary:
Fixes T4366. Two years ago, Facebook put 16,000 files in a directory. Today, the page has nearly loaded.
Paginate large directories.
Test Plan:
- Viewed home and browse views in Git, Mercurial and Subversion.
I put an artificially small page size (5) on home:
{F1055653}
I pushed 16,000 files to a directory and paged through them. Here's the last page, which rendered in about 200ms:
{F1055655}
Our behavior is a bit better than GitHub here, which shows only the first 1,000 files, disables pagination, and can't retrieve history for the files:
{F1055656}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4366
Differential Revision: https://secure.phabricator.com/D14956
Summary: Make sure to subclass the right controller on badges.
Test Plan: arc liberate, make a custom badges edit form.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14961
Summary:
When looking at a large file in Diffusion:
- disable highlighting if it's huge and show a note about why;
- pick up a few other optimizations.
Test Plan: Locally, this improves the main render of `__phutil_library_map__.php` from 3,200ms to 600ms for me, at the cost of syntax highlighting (we can eventually add view options and let users re-enable it).
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14959
Summary:
Ref T2450. Ref T9319. This is still a bit messy, but not quite so bad as it was: instead of using a single call to get both blame information and file content, use `diffusion.blame` for blame information.
This will make optimizations to both blame and file content easier.
Test Plan: Viewed a bunch of blame (color on/off, blame on/off).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2450, T9319
Differential Revision: https://secure.phabricator.com/D14958
Summary:
Fixes T2451. Several motivations here, from strongest to weakest:
- Currently, getting blame and file content are closely entwined. This makes fixing T9319 more difficult, and I want to fix it. I want to separate blame from content so there's more flexibility in how we approach this issue.
- This makes pursuing T2450 easier, if it turns out to be a meaningful win.
- If we can get a win on blame performance, we can do `arc blame` eventually if we want.
Test Plan:
- Blamed in SVN, Git and Mercurial.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2451
Differential Revision: https://secure.phabricator.com/D14957
Summary:
Fixes T10089. This did work at one point, but was broken by D12868, which got too aggressive about mailing members.
We don't want to send mail to all members by default, only those who are subscribed. The parent implementation of `getMailCC()` handles this for us.
Test Plan:
Joined a project as users A and B. Unsubscribed with B. Made an edit.
Before patch: both A and B got mail. After patch: only A got mail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10089
Differential Revision: https://secure.phabricator.com/D14955
Summary: Adds a basic HeraldAdapter to Phame Blogs and Posts.
Test Plan: Make a Herald rule to CC me on new posts or blogs automatically.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14897
Summary: Fixes T9191. This is pretty fluff but doesn't hurt anything, I guess.
Test Plan: Viewed repository list, saw an importing repository get a little icon.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9191
Differential Revision: https://secure.phabricator.com/D14950
Summary: Ref T4245. This is the last of it, and covers the clone/push stuff.
Test Plan:
- Cloned git.
- Pushed git.
- Cloned mercurial.
- Pushed mercurial.
- Visited a `blah.git` URL in my browser just because; got redirected to a human-facing UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14949
Summary: Ref T4245. On their best day these don't work all that well, but I'm pretty sure I didn't make anything worse.
Test Plan:
- Viewed global lint.
- Viewed lint for a repository.
- Viewed lint details for a particular message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14948
Summary: Ref T4245. This gets everything else except serving HTTP requests (complicated) and lint (quite weird).
Test Plan:
- Viewed a diff.
- Viewed externals.
- Viewed history table to see last modified.
- Did path completion and validation in Owners.
- Did tree path search in Diffusion.
- Viewed a repository.
- Created a new repository.
- Looked up symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14947
