Summary:
Ref T4788. Fixes T10703.
In the longer term I want to put this on top of ApplicationSearch, but that's somewhat complex and we're at a fairly good point to pause this feature for feedback.
Inch toward that instead: provide more appropriate filters and defaults without rebuilding the underlying engine. Specifically:
- No "assigned" for commits (barely makes sense).
- No "assigned" for mocks (does not make sense).
- Default to "open" for parent tasks, subtasks, close as duplicate, and merge into.
Also, add a key to the `search_document` table to improve the performance of the "all open stuff of type X" query. "All Open Tasks" is about 100x faster on my machine with this key.
Test Plan:
- Clicked all object relationships, saw more sensible filters and defaults.
- Saw "open" query about 100x faster locally (300ms to 3ms).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T10703
Differential Revision: https://secure.phabricator.com/D16202
Summary: Fixes T11240. Also simplify things a little and share a bit more code.
Test Plan:
- Viewed revisions and tasks, opened submenu.
- Viewed as a user without edit permission, saw the menus greyed out.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11240
Differential Revision: https://secure.phabricator.com/D16201
Summary:
Ref T4788. Fixes T7820. This updates the "Merge Duplicates In" interaction, and adds a "Close as Duplicate" action.
These are the last interactions that were using the old code, so it removes that code.
Merges are now recorded as real edges, so we can show them in the UI later on (originally from T9390, etc).
Also provides more general support for relationships which need EDIT permission, not-undoable relationships like merges, preventing relating an object to itself, and relationship side effects like merges.
Finally, fixes a couple of behaviors around typing an exact object name (like `T123`) to find the related object.
Test Plan:
- Merged tasks into the current task.
- Closed the current task as a duplicate of another task.
- Edited other relationships.
- Searched for tasks, commits, etc., by object monogram.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T7820
Differential Revision: https://secure.phabricator.com/D16196
Summary:
The first dialog was being given the wrong user (`$user`, should be `$viewer`), leading to a CSRF issue.
(The CSRF token it generated was invalid in all validation contexts, so this wasn't a security problem or a way to capture CSRF tokens for other users.)
Use `newDialog()` instead.
(This seems completely unrelated to the vaguely-similar-looking issues we saw earlier this week.)
Test Plan:
- Added a new email address.
- Clicked "Done" on the last step.
- Completed workflow instead of getting a CSRF error.
Reviewers: chad, tide
Reviewed By: tide
Differential Revision: https://secure.phabricator.com/D16200
Summary:
Ref T9360.
[x] View Live useless on archived blogs
[x] Edit Blog Image treatment like profiles
[x] Pager next/prev should keep you on whatever view you're on
[x] Unset user titles aren't falling back properly
[x] Add captions to edit fields for better clarification
Test Plan: Archive a blog, Edit a photo, verify pager on live and internal blogs, check empty titles, and view new edit form instructions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16197
Summary: Ref T4788. This moves everything except "merge" to the new code.
Test Plan:
- Edited relationships in Differential, Diffusion, and Pholio.
- Uninstalled Pholio, made sure "Edit Mocks..." actions vanished.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16193
Summary:
Ref T4788. Fixes T9232. This moves the "search for stuff to attach to this object" flow away from hard-coding and legacy constants and toward something more modular and flexible.
It also adds an "Edit Commits..." action to Maniphest, resolving T9232. The behavior of the search for commits isn't great right now, but it will improve once these use real ApplicationSearch.
Test Plan: Edited a tasks' related commits, mocks, tasks, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T9232
Differential Revision: https://secure.phabricator.com/D16189
Summary:
Ref T11232. The cluster connection pathway specifies a timeout when connecting, but this connection pathway does not. (I'm not sure if we just never did or if it got lost at some point.)
Soon, T11044 will obsolete this and unify the database connection pathways, but that's a more complicated change.
I'm not sure if this will fix T11232, but it can't hurt.
Test Plan: Put a `throw` on timeout specifications. Before the change: did not hit it in non-cluster configurations. After the change: hit it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11232
Differential Revision: https://secure.phabricator.com/D16194
Summary: Fixes T11223. I missed a few of these; most of them kept working anyway because we have redirects in place, but make them a bit more modern/not-hard-coded.
Test Plan:
- Generated and revoked API tokens for myself.
- Generated and revoked API tokens for bots.
- Revoked temporary tokens for myself.
- Clicked the link to the API tokens panel from the Conduit console.
- Clicked all the cancel buttons in all the dialogs, too.
In all cases, everything now points at the correct URIs. Previously, some things pointed at the wrong URIs (mostly dealing with stuff for bots).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11223
Differential Revision: https://secure.phabricator.com/D16185
Summary: love to wordsmith
Test Plan: read it
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16183
Summary:
Ref T9838.
Add a Properties field to Revision, and update a `wasAcceptedBeforeClose` when closing a revision.
Test Plan:
A quick run through the obvious steps (Close with commit/manually, with or w/o accept) and calling `differential.query` shows the `wasAcceptedBeforeClose` property was setup correctly.
Pushing closed + accepted passes the relevant herald, which was my immediate issue; Pushing un-accepted is blocked.
Test the "commit" rule (Different from "pre-commit") by hacking the DB and running the "has accepted revision" rule in a test-console.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T9838
Differential Revision: https://secure.phabricator.com/D15085
Summary: Ref T9897. This moves "Domain" to "DomainFullURI" to allow setting of https or for some reason, a port. I guess.
Test Plan: Try to break by setting a path, or fake protocol. Set to http, or https, see correct redirects. Verify domain still gets written.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16173
Summary:
Ref T11208. See that task for a more detailed description of revprops.
This allows revprop changes in a hosted Subversion repository if the repository has the "allow dangerous changes" flag set.
In the future, we could expand this into real Herald support, but the only use case we have for now is letting `svnsync` work.
Test Plan:
Edited revprops with `svn propset --revprop -r 2 propkey propvalue repositoryuri`:
- Tried before patch, got a "configure a commit hook" error.
- Tried after patch, got a "dangerous change" error.
- Allowed dangerous changes.
- Did a revprop edit.
- Prevented dangerous changes.
- Got an error again.
- Made a normal commit to an SVN repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11208
Differential Revision: https://secure.phabricator.com/D16174
Summary: Fixes T11201.
Test Plan:
Created bogus edges like this:
```
INSERT INTO edge (src, type, dst, dateCreated, seq) values ('PHID-TASK-vnddativbialb5p6ymis', 999999, 'quack', UNIX_TIMESTAMP(), 1);
```
Then ran `bin/remove destroy` on the relevant object.
Before the patch, destruction halted after hittin the bad edge.
After the patch, a warning is emitted but destruction continues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11201
Differential Revision: https://secure.phabricator.com/D16171
Summary: Fixes T11164. At least, this fixes it locally for me. I don't know how to code. Copy Pasta!
Test Plan: Change name, don't see extra timeline entry on quality set anymore.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11164
Differential Revision: https://secure.phabricator.com/D16169
Summary: Fixes T11166. Adds some class, and space to the preview widget.
Test Plan: Test Maniphest, Ponder, etc, without a footer.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11166
Differential Revision: https://secure.phabricator.com/D16168
Summary: Fixes T11198. These are confusing or premature if you aren't an activated user: disabled or unapproved accounts won't be able to act on them.
Test Plan: Changed timezone, went through flow to correct it
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11198
Differential Revision: https://secure.phabricator.com/D16167
Summary:
Ref T11179. This splits "Edit Blocking Tasks" into two options now that we have more room ("Edit Parent Tasks", "Edit Subtasks").
This also renames "Blocking" tasks to "Subtasks", and "Blocked" tasks to "Parent" tasks. My goals here are:
- Make the relationship direction more clear: it's more clear which way is up with "parent" and "subtask" at a glance than with "blocking" and "blocked" or "dependent" and "dependency".
- Align language with "Create Subtask".
- To some small degree, use more flexible/general-purpose language, although I haven't seen any real confusion here.
Fixes T6815. I think I narrowed this down to two issues:
- Just throwing a bare exeception (we now return a dialog explicitly).
- Not killing open transactions when the cyclec check fails (we now kill them).
Test Plan:
- Edited parent tasks.
- Edited subtasks.
- Tried to introduce graph cycles, got a nice error dialog.
{F1697087}
{F1697088}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6815, T11179
Differential Revision: https://secure.phabricator.com/D16166
Summary:
Ref T11179. Ref T4768. Currently, on `master`, if two users open "Edit Revisions" at the same time, then add revisions A and B, only the last state wins (just "B").
Instead, apply these as "add A" and "add B" so they merge in a natural way.
Test Plan:
- Opened edit dialog in two windows.
- Added "A" in one, "B" in the other.
- Saved both.
- Saw "Added A" and "Added B" transactions, instead of "Added A" and "Removed A, added B".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4768, T11179
Differential Revision: https://secure.phabricator.com/D16164
Summary: Ref T11179. This is basically a "pro" controller to replace the SearchAttach controller. It does basically the same stuff, just in a (mostly) more modern and modular way.
Test Plan:
- Added and removed mocks.
- Added and removed revisions.
- Everything worked just like it did before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16163
Summary:
Ref T11179. This generates the Maniphest menu items in a modular way. It doesn't change any of the underlying code yet.
Searching for commits doesn't work particularly well so I've just hidden that for now, but the item itself works fine.
Test Plan: {F1696849}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16162
Summary: Fixes T11193. Assume this is the correct place to check for permissions before attaching edges.
Test Plan: Create a task and set edit policy to Admins, log into test account. Try to Edit Subtasks, Merge Duplicates, Attach a Diff, or Attach a Mock, get a Policy Dialog explaing why.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11193
Differential Revision: https://secure.phabricator.com/D16161
Summary: Fixes T11190. The div with all the stuff in it was sometimes ending up on top of the "select" button, making it unclickable.
Test Plan: Clicked "select" in several browsers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11190
Differential Revision: https://secure.phabricator.com/D16160
Summary: This makes it more natural to write Herald rules about commits that appear on any or no branches.
Test Plan: Wrote a commit rule for commits on any branch, ran it with `bin/repository reparse --herald <commit>`, saw expected results in web UI.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16158
Summary:
Ref T11179. Alternative to D16152. I think this turned out a bit better than the other one did.
Currently, we render two copies of the menu (one for mobile, one for desktop). A big chunk of this is sharing the nodes instead: when you open the mobile dropdown menu, it steals the nodes from the document. When you close it, it puts them back. Magic! Sneaky!
Test Plan:
{F1695499}
{F1695500}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16157
Summary: Ref T11034. Ref T4788. This allows you to resize the typeahead browse dialog if you want. I plan to let you resize the object selector dialog in the future.
Test Plan: {F1695433}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T11034
Differential Revision: https://secure.phabricator.com/D16156
Summary:
Ref T11034. This seems a little more promising. Two problems at the moment:
- This doesn't actually provide any useful information at all right now.
- Many object types have no profile images.
Test Plan:
{F1695254}
{F1695255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11034
Differential Revision: https://secure.phabricator.com/D16155
Summary:
Ref T11179. One issue I'm getting with trying to turn actions into dropdowns is that we currently render this menu very late, which can cause us to try to add more metadata after we start resolving metadata. This won't work right now (and making it work seems unreasonably complicated), so stop doing it and fatal if something tries.
(This might make some things fatal but //should// be safe -- anything that fatals should have been broken already.)
Test Plan:
Browsed around looking for fatals, didn't see any.
(This primarily avoids a broken state / fatal in a future diff.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16151
Summary: Ref T9897. Adds a Parent Site and Parent Domain field to allow external sites to link back to parent.
Test Plan: Set up ```local.blog.phacility.com```, set parent site to "Phacility" and parent domain to "local.www.phacility.com". Get new crumbs at Blog and Post levels.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16150
Summary:
Fixes T11180. In Git, it's possible to tag a tag (????). When you do, we try to log the tag-object, which automatically resolves to the commit and fails.
Just skip these. If "A" points at "B" which points at "C", it's fine to ignore "A" and "B" since we'll get the same stuff when we process "C".
Test Plan:
- Tagged a tag.
- Pushed it.
- Discovered it.
- Before patch: got exception similar to the one in T11180.
- After patch: got tag-tag skipped. Also got slightly better error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11180
Differential Revision: https://secure.phabricator.com/D16149
Summary: Reading the code, this seems correct, but I don't have a local test. Ref T9897
Test Plan: read carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16148
Summary: Makes the crumbs background and border disappear in the live view of Phame.
Test Plan: Go live, see no crumb bg. Test blog, post, mobile, desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16146
Summary: Adds a new header layout for Phame Blog. Subtitles now also.
Test Plan:
With Image, With Subtitle, Without Image, Without Subtitle. Mobile, Tablet, Desktop.
{F1691506}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16147
Summary:
Ref T11153. If you have a build plan like this:
- Lease machine A.
- Lease machine B.
- Run client-tests on machine A.
- Run server-tests on machine B.
...and we get machine A quickly, then finish the tests, we currently do not release machine A until the whole plan finishes.
In the best case, this wastes resources (something else could be using that machine for a while).
In a worse case, this wastes a lot of resources (if machine B is slow to acquire, or the server tests are much slower than the client tests, machine A will get tied up for a really long time).
In the absolute worst case, this might deadlock things.
Instead, release artifacts as soon as no waiting/running steps take them as inputs. In this case, we'd release machine A as soon as we finished running the client tests.
In the case where machines A and B are resources of the same type, this should prevent deadlocks. In all cases, this should improve build throughput at least somewhat.
Test Plan:
I wrote this build plan which runs a "fast" step (10 seconds) and a "slow" step (120 seconds):
{F1691190}
Before the patch, running this build plan held the lease on the "fast" machine for the full 120 seconds, then released both leases at the same time at the very end.
After this patch, I ran this plan and observed the "fast" lease get released after 10 seconds, while the "slow" lease was held for the full 120.
(Also added some `var_dump()` into things to sanity check the logic; it appeared correct.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11153
Differential Revision: https://secure.phabricator.com/D16145
Summary: Adds a view live and view internal link to the blog and crumbs manage page.
Test Plan: Click on new links.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16142
Summary: Fixes T10901. Allows blogs to have headers. I've built this in a basic way, any file, max-height is 240. Should bleed into top crumbs, so any spacing you want you should add to the file itself. Might have to see how users break this.
Test Plan: Set a blog header, see blog header, remove blog header, see no blog header. Check mobile, tablet, desktop break points.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10901
Differential Revision: https://secure.phabricator.com/D16141
Summary: This is the backend half of uploading an image as a header for Phame Blogs. Allows you to upload image, or delete it. Ref T10901
Test Plan:
Go to Manage Blog, visit Edit Header Image, Upload snarky file. See snarky file on Manage page. Edit Header Image, click delete, save, see file goes away.
{F1690966}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10901
Differential Revision: https://secure.phabricator.com/D16140
Summary:
This test has been failing occasionally in a way that does not reproduce, and only when no one is looking at it.
Try to add some extra assertions to maybe get more information.
Test Plan: `arc unit`
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16137
Summary: Ref T9028. When selecting refs, pretend refs in "refs/remotes/" that we don't otherwise recognize don't exist, since it looks like these are probably remotes //of the remote// we're observing, and who knows what state they're in.
Test Plan: Used `bin/repository discover --verbose` to verify that these named refs no longer appear in the list.
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16136
Summary:
Ref T9028. Mostly, this gives them a strikethru style.
(I think this is probably the right definition of "closed" for commits. Another definition might be "audited", but I don't think completing audits really "closes" a commit.)
Test Plan: {F1689662}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16135
Summary: Ref T9028. Ref T6878. This rule should probably be refined in the long term, but for now just ignore "phabricator/diff/12424" and similar staging area tags.
Test Plan: Ran `bin/repository discover --verbose` on a repository with staging area refs, saw Phabricator ignore those refs as untracked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6878, T9028
Differential Revision: https://secure.phabricator.com/D16134
Summary:
Ref T9028. This allows us to detect when commits are unreachable:
- When a ref (tag, branch, etc) is moved or deleted, store the old thing it pointed at in a list.
- After discovery, go through the list and check if all the stuff on it is still reachable.
- If something isn't, try to follow its ancestors back until we find something that is reachable.
- Then, mark everything we found as unreachable.
- Finally, rebuild the repository summary table to correct the commit count.
Test Plan:
- Deleted a ref, ran `pull` + `refs`, saw oldref in database.
- Ran `discover`, saw it process the oldref, mark the unreachable commit, and update the summary table.
- Visited commit page, saw it properly marked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16133
Summary:
Ref T9028. This corrects the reachability of existing commits in a repository.
In particular, it can be used to mark deleted commits as unreachable.
Test Plan:
- Ran it on a bad repository, with bad args, etc.
- Ran it on a clean repo, got no changes.
- Marked a reachable commit as unreachable, ran script, got it marked reachable.
- Started deleting tags and branches from the local working copy while running the script, saw greater parts of the repository get marked unreachable.
- Pulled repository again, everything automatically revived.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16132
Summary:
Ref T9028. This improves the daemon behavior for unreachable commits. There is still no way for commits to become marked unreachable on their own.
- When a daemon encounters an unreachable commit, fail permanently.
- When we revive a commit, queue new daemons to process it (since some of the daemons might have failed permanently the first time around).
- Before doing a step on a commit, check if the step has already been done and skip it if it has. This can't happen normally, but will soon be possible if a commit is repeatedly deleted and revived very quickly.
- Steps queued with `bin/repository reparse ...` still execute normally.
Test Plan:
- Used `bin/repository reparse` to run every step, verified they all mark the commit with the proper flag.
- Faked the `reparse` exception in the "skip step" code, used `repository reparse` to skip every step.
- Marked a commit as unreachable, ran `discover`, saw daemons queue for it.
- Ran daemons with `bin/worker execute --id ...`, saw them all skip + queue the next step.
- Marked a commit as unreachable, ran `bin/repository reparse` on it, got permanent failures immediately for each step.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16131
Summary:
Ref T9028. This is the easy part of dealing with deleted commits:
- Add a flag for unreachable commits (nothing sets this flag yet).
- Ignore unreachable commits when querying for known commits during discovery, so we pretend they do not exist.
- When recording a commit, try just reviving an existing unreachable commit first. If that works, bail out.
Test Plan:
- Artificially marked a commit as unreachable with raw SQL.
- Verified it said "deleted: unreachable" in the UI.
- Ran `repository discover --trace --verbose`.
- Saw the discovery process ignore the commit when filling the cache.
- Saw the discovery process revive the commit instead of trying to record it again.
- Web UI now shows the commit as normal.
- Running `repository discover` again doesn't make any further changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16130
Summary:
Ref T9028. Fixes T6878. Currently, we only fetch and discover branches. This is fine 99% of the time but sometimes commits are pushed to just a tag, e.g.:
```
git checkout <some hash>
nano file.c
git commit -am '...'
git tag wild-wild-west
git push origin wild-wild-west
```
Through a similar process, commits can also be pushed to some arbitrary named ref (we do this for staging areas).
With the current rules, we don't fetch tag refs and won't discover these commits.
Change the rules so:
- we fetch all refs; and
- we discover ancestors of all refs.
Autoclose rules for tags and arbitrary refs are just hard-coded for now. We might make these more flexible in the future, or we might do forks instead, or maybe we'll have to do both.
Test Plan:
Pushed a commit to a tag ONLY (`vegetable1`).
<cf508b8de6>
On `master`, prior to the change:
- Used `update` + `refs` + `discover`.
- Verified tag was not fetched with `git for-each-ref` in local working copy and the web UI.
- Verified commit was not discovered using the web UI.
With this patch applied:
- Used `update`, saw a `refs/*` fetch instead of a `refs/heads/*` fetch.
- Used `git for-each-ref` to verify that tag fetched.
- Used `repository refs`.
- Saw new tag appear in the tags list in the web UI.
- Saw new refcursor appear in refcursor table.
- Used `repository discover --verbose` and examine refs for sanity.
- Saw commit row appear in database.
- Saw commit skeleton appear in web UI.
- Ran `bin/phd debug task`.
- Saw commit fully parse.
{F1689319}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T6878, T9028
Differential Revision: https://secure.phabricator.com/D16129
Summary:
Ref T11140. This makes encryption actually work:
- Provide a new configuation option, `keyring`, for specifying encryption keys.
- One key may be marked as `default`. This activates AES256 encryption for Files.
- Add `bin/files generate-key`. This is helps when generating valid encryption keys.
- Add `bin/files encode`. This changes the storage encoding of a file, and helps test encodings and migrate existing data.
- Add `bin/files cycle`. This re-encodes the block key with a new master key, if your master key leaks or you're just paraonid.
- Document all these options and behaviors.
Test Plan:
- Configured a bad `keyring`, hit a bunch of different errors.
- Used `bin/files generate-key` to try to generate bad keys, got appropriate errors ("raw doesn't support keys", etc).
- Used `bin/files generate-key` to generate an AES256 key.
- Put the new AES256 key into the `keyring`, without `default`.
- Uploaded a new file, verified it still uploaded as raw data (no `default` key yet).
- Used `bin/files encode` to change a file to ROT13 and back to raw. Verified old data got deleted and new data got stored properly.
- Used `bin/files encode --key ...` to explicitly convert a file to AES256 with my non-default key.
- Forced a re-encode of an AES256 file, verified the old data was deleted and a new key and IV were generated.
- Used `bin/files cycle` to try to cycle raw/rot13 files, got errors.
- Used `bin/files cycle` to cycle AES256 files. Verified metadata changed but file data did not. Verified file data was still decryptable with metadata.
- Ran `bin/files cycle --all`.
- Ran `encode` and `cycle` on chunked files, saw commands fail properly. These commands operate on the underlying data blocks, not the chunk metadata.
- Set key to `default`, uploaded a file, saw it stored as AES256.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16127
Summary:
Ref T11140. This doesn't do anything yet since there's no way to enable it and no way to store master keys.
Those are slightly tougher problems and I'm not totally satisfied that I have an approach I really like for either problem, so I may wait for a bit before tackling them. Once they're solved, this does the mechanical encrypt/decrypt stuff, though.
This design is substantially similar to the AWS S3 server-side encryption design, and intended as an analog for it. The decisions AWS has made in design generally seem reasonable to me.
Each block of file data is encrypted with a unique key and a unique IV, and then that key and IV are encrypted with the master key (and a distinct, unique IV). This is better than just encrypting with the master key directly because:
- You can rotate the master key later and only need to re-encrypt a small amount of key data (about 48 bytes per file chunk), instead of re-encrypting all of the actual file data (up to 4MB per file chunk).
- Instead of putting the master key on every server, you can put it on some dedicated keyserver which accepts encrypted keys, decrypts them, and returns plaintext keys, and can send it 32-byte keys for decryption instead of 4MB blocks of file data.
- You have to compromise the master key, the database, AND the file store to get the file data. This is probably not much of a barrier realistically, but it does make attacks very slightly harder.
The "KeyRing" thing may change once I figure out how I want users to store master keys, but it was the simplest approach to get the unit tests working.
Test Plan:
- Ran unit tests.
- Dumped raw data, saw encrypted blob.
- No way to actually use this in the real application yet so it can't be tested too extensively.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16124
Summary: Fixes T11156. These were never correct, but also never actually used until I made timelines load object handles unconditionally in D16111.
Test Plan: Viewed an auth provider with transactions, no more fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11156
Differential Revision: https://secure.phabricator.com/D16128
Summary: Uses PHUITwoColumnView in Blog Manage and Blog Picture. Ref T9897
Test Plan: Use each page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16126
Summary: Adds some ordering options to PhamePost queries. Works on search, PhameHome, BlogHome
Test Plan: Try searching with Order By set to Date Published in application search, get correct order. Check a blog home page, check PhameHome.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16125
Summary:
Ref T11140. When reading and writing files, we optionally apply a "storage format" to them.
The default format is "raw", which means we just store the raw data.
This change modularizes formats and adds a "rot13" format, which proves formatting works and is testable. In the future, I'll add real encryption formats.
Test Plan:
- Added unit tests.
- Viewed files in web UI.
- Changed a file's format to rot13, saw the data get rotated on display.
- Set default format to rot13:
- Uploaded a small file, verified data was stored as rot13.
- Uploaded a large file, verified metadata was stored as "raw" (just a type, no actual data) and blob data was stored as rot13.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16122
Summary: Ref T11142. H264 video in a Quicktime container works in Safari and Firefox for me (although not Chrome), so include it in the default video mime types.
Test Plan: Uploaded video file from T11142 locally, saw it render with `<video />` properly in Safari.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11142
Differential Revision: https://secure.phabricator.com/D16121
Summary: Flips the bits from true to false in transaction editor.
Test Plan: update a post, search for new term
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16120
Summary: Ref T9897, makes blogs searchable
Test Plan: Make a blog, index it, search for it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16119
Summary:
Ref T11098. Mixture of issues here:
- Similar problem to D16112, where users with no settings at all could fail to fall back to the global defaults.
- I made `UserPreferencesQuery` responsible for building defaults instead to simplify this, since we have 4 or 5 callsites which need to do it and they aren't easily reducible.
- Handle cases where `metamta.one-mail-per-recipient` is off (and thus users can not have any custom settings) more explicitly.
- When `metamta.one-mail-per-recipient` is off, remove the "Email Format" panel for users only -- administrators can still access it in global preferences.
Test Plan:
- Deleted a user's preferences, changed globals, purged cache, made sure defaults reflected global defaults.
- Changed global mail tags, sent mail to the user, verified it was dropped in accordinace with global settings.
- Changed user's settings to get the mail instead, verified mail was sent.
- Toggled user's Re / Vary settings, verified mail subject lines reflected user settings.
- Disabled `metamta.one-mail-per-recipient`, verified user "Email Format" panel vanished.
- Edited "Email Format" in single-mail-mode in global prefs as an administrator.
- Sent more mail, verified mail respected new global settings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16118
Summary: Ref T11098. There is no reason to maintain these as separate values now that they can be configured in global settings.
Test Plan:
- Hit and read setup issue.
- Fiddled with settings.
- I'll vet this more throughly in the next diff since I need to fix an issue with global defaults in mail and can explicitly test this at the same time.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16117
Summary: Adds PhamePost object to fulltextsearch index. Some issue searching just "Open" though? Also "closed" objects search fine but don't display as disabled.
Test Plan:
bin/search index --type POST
{F1687043}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16116
Summary: Ref T9789. Falling back to `parent::` is better, and fixes older-style feed stories for Pastes, like "added a comment".
Test Plan: Viewed a comment feed story about a paste.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16114
Summary:
The option names for `Vary Subjects` are copypasta from the `Add "Re:" Prefix` option. Fix their names to refer to `Vary Subjects` instead.
Fixes T11148
Test Plan: Verify option names for `Vary Subjects` refer to `Add "Re:" Prefix` before apply. Verify they no longer do after apply.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T11148
Differential Revision: https://secure.phabricator.com/D16113
Summary:
Ref T9789. `Transaction` and `Editor` classes are the last major pieces of infrastructure that haven't been fully modularized.
Some of the specific issues are:
- `Editor` classes rely on a bunch of `instanceof` stuff in the base class to pick up transaction types like "subscribe", "projects", etc. Instead, applications should be adding these, and third-party applications should be able to add them.
- Code is spread across `Transaction` and `Editor` classes somewhat oddly. For example, generating old/new values would probably make more sense at the `Transaction` level, but it currently exists at the `Editor` level.
- Both types of classes have a lot of functions based on `switch()` statements, which require a ton of boilerplate and are just generally kind of hard to work with.
This creates classes for each type of transaction, and moves almost all of the logic to them. These classes are simpler and more focused than the old stuff was, and can organize related code better.
This starts inching toward defining `CoreTransactions` for features shared across applications. It only defines the "Create" transaction so far, but at some point I plan to move all the other shared transactions to Core and let them control which objects they're available for.
Test Plan:
- Created pastes with web UI and API.
- Edited all paste properites.
- Archived/activated.
- Verified files got reasonable names.
- Reviewed timeline and feed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16111
Summary:
Ref T11098. Users with at least one setting set correctly fall back to the defaults, but users with no settings at all currently do not.
Make them fall back to global defaults properly.
Test Plan:
- Set global defaults to some non-default setting.
- Completely delete a user's settings.
- `bin/cache purge --purge-all` or `--purge-user`.
- View settings as the user.
- Before change: showed hard-coded defaults instead of global defaults until you save anything.
- After change: properly shows global defaults from the start.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16112
Summary:
Ref T11035. This only fixes half of the issue: comment editing has been fixed, but normal transactions which edit things like descriptions haven't yet.
The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order.
Test Plan:
- Mentioned `@dog` in a comment.
- Removed `@dog` as a subscriber.
- Edited the comment, adding some unrelated text at the end (e.g., fixing a typo).
- Before change: `@dog` re-added as subscriber.
- After change: no re-add.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11035
Differential Revision: https://secure.phabricator.com/D16108
Summary: Adds a quick edit link to PhamePosts in ApplicationSearch
Test Plan: Review a few searches, click on pencil.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16109
Summary:
When having lots of repos, seeing "all revisions in this project" is hard, and we ended up adding herald rules to basically copy project tags to the revisions on a per-project basis. Adding a "tagged: project" function to the Repositories search field allows users to find differentials within a project.
Fix T10850.
Test Plan: search differentials by tagging project and repository in the Repository field
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: avivey, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10850
Differential Revision: https://secure.phabricator.com/D16096
Summary: Forgot to save this file locally. Adds isArchived to same hidden features as isDraft
Test Plan: test mail on archived posts
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16106
Summary: Ref T9897. Adds ability to Archive a Phame Post (only visible under ApplicationSearch).
Test Plan: Archive a post, re-publish it, search for it, archive it again. View Home, Blog, Live pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16104
Summary: Fixes T11139. We missed this years ago when we moved to PhutilUTF8StringTruncator.
Test Plan: {F1686072}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11139
Differential Revision: https://secure.phabricator.com/D16105
Summary:
Ref T11137. This class is removed in D16099. Depends on D16099.
`PhutilURI` now attempts to "just work" with Git-style URIs, so at least in theory we can just delete all of this code and pretend it does not exist.
(I've left "Display URI" and "Effective URI" as distinct, at least for now, because I think the distinction may be relevant in the future even though it isn't right now, and to keep this diff small, although I may go remove one after I think about this for a bit.)
Test Plan:
- Created a new Git repository with a Git URI.
- Pulled/updated it, which now works correctly and should resolve the original issue in T11137.
- Verified that daemons now align the origin to a Git-style URI with a relative path, which should resolve the original issue in T11004.
- Grepped for `PhutilGitURI`.
- Also grepped in `arcanist/`, but found no matches, so no patch for that.
- Checked display/conduit URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11137
Differential Revision: https://secure.phabricator.com/D16100
Summary:
Ref T7643. When a large block of prose text is edited (like a wiki page), summarize the diff when sending mail.
For now, I'm still showing the whole thing in the web UI, since it's a bit more manageable there.
Also try to fix newlines in Airmail.
Test Plan:
This web diff:
{F1682591}
..became this mail diff:
{F1682592}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16098
Summary:
Ref T8510. Sort prefix matches above non-prefix matches, so that "Ape Discovery" does not match "discovery" better than "Discovery".
Sort functions last.
Rename function internal strings so they don't get over-promoted the prefix-match rules.
Add kind of a hack to get "Project X" sorting above all the "Project X (Milestone 1)" results.
Test Plan:
Created "Ape Discovery", "Baboon Discovery", "Chimpanzee Discovery", etc.
Main project now sorts above milestones:
{F1681773}
Prefix matches now sort above other matches:
{F1681774}
Function results (rarely used) are now less prominent:
{F1681775}
Better function results here:
{F1681776}
More function results:
{F1681777}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8510
Differential Revision: https://secure.phabricator.com/D16094
Summary: Ref T11120. If this works, I'll just remove this option completely.
Test Plan: ¯\_(ツ)_/¯
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11120
Differential Revision: https://secure.phabricator.com/D16095
Summary: "All Tasks" is bad in the long run and not clearly better for new installs.
Test Plan: Created a new smiple template, saw open tasks only.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16093
Summary: Ref T10227. When we perform `git` http operations (fetch, mirror) check if we should use a proxy; if we should, set `http_proxy` or `https_proxy` in the environment to make `git` have `curl` use it.
Test Plan:
- Configured a proxy extension to run stuff through a local instance of Charles.
- Ran `repository pull` and `repository mirror`.
- Saw `git` HTTP requests route through the proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10227
Differential Revision: https://secure.phabricator.com/D16092
Summary: Ref T11123. This implements a very basic skeleton for modern revision search.
Test Plan: Viewed and executed Conduit API method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11123
Differential Revision: https://secure.phabricator.com/D16089
Summary: Ref T6523. Allows you to click stuff instead of using drag-and-drop.
Test Plan: On iOS simulator, created and updated a mock.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6523
Differential Revision: https://secure.phabricator.com/D16088
Summary: Fixes T10886. This should get more formal some day, but just fix it for now.
Test Plan: Reloaded mock with other unpublished draft inlines, saw accurate count.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10886
Differential Revision: https://secure.phabricator.com/D16087
Summary: Fixes T11115, but unclear how to test this. I think I've asked this in the past.
Test Plan:
- Visit Applications -> Ponder
- Configure external email
- Test External Email
- See new Question
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11115
Differential Revision: https://secure.phabricator.com/D16084
Summary: Ref T4280. At some point (probably D15732) we started getting anchor parsing wrong. Just pop the anchor off before doing all the logic, then put it back on at the end.
Test Plan:
Tested various forms like:
```
[[ x ]]
[[ x | z ]]
[[ x#y | z ]]
[[ ./x#y | z ]]
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4280
Differential Revision: https://secure.phabricator.com/D16083
Summary: Fixes T11113. On the 2nd+ page, we could end up with an ambiguous `id` WHERE clause because we don't define a primary table alias on this query. Define one.
Test Plan:
Changed SearchEngine to return pages of size 5, searched for my threads, toggled to second page, no exception.
Used DarkConsole to examine that second-page query, saw that it had `thread.id` explicitly instead of `id` implicitly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11113
Differential Revision: https://secure.phabricator.com/D16080
Summary:
Via HackerOne. This page fatals if accessed directly while logged out.
The "shouldRequireLogin()" check is wrong; this is a logged-in page.
Test Plan:
Viewed the page while logged out, no more fatal.
Faked my way through the actual verification flow.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16077
Summary: Fixes T11107. The URI change here meant we were dropping the "key" parameter, which allows you to set a new password without knowing your old one.
Test Plan: Reset password, didn't need to provide old one anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11107
Differential Revision: https://secure.phabricator.com/D16075
Summary: Ref T6916. Added video to remarkup using D7156 as reference.
Test Plan:
- Viewed video files (MP4, Ogg) in Safari, Chrome, Firefox (some don't work, e.g., OGG in Safari, but nothing we can really do about that).
- Used `alt`.
- Used `autoplay`.
- Used `loop`.
- Used `media=audio`.
- Viewed file detail page.
Reviewers: nateguchi2, chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: asherkin, ivo, joshuaspence, Korvin, epriestley
Tags: #remarkup
Maniphest Tasks: T6916
Differential Revision: https://secure.phabricator.com/D11297
Summary:
Fixes T10402.
I tried about 50 variations on the wording and notification layout, this seemed by far the most reasonable.
Didn't implement a way to ignore the warning, which might be required - but figured this is serious and broken enough while being completely invisible 99% of the time that it's worth shouting about.
Test Plan: Messed around with $_SERVER['HTTPS'] on the server side and client_uri on the client side - saw reasonable results in all combinations.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10402
Differential Revision: https://secure.phabricator.com/D16064
Summary:
Ref T11098. This primarily fixes Conduit calls to `*.edit` methods failing when trying to access user preferences.
(The actual access is a little weird, since it seems like we're building some UI stuff inside a policy query, but that's an issue for another time.)
To fix this, consolidate the "we're about to run some kind of request with this user" code and run it consistently for web, conduit, and SSH sessions.
Additionally, make sure we swap things to the user's translation.
Test Plan:
- Ran `maniphest.edit` via `arc call-conduit`, no more settings exception.
- Set translation to ALL CAPS, got all caps output from `ssh` and Conduit.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16066
Summary:
Ran into this while fixing T11098#179088.
The "Transaction Type" details in the conduit autogenerated documentation for `*.edit` endpoints still wraps incorrectly.
Test Plan: Purged remarkup cache, reloaded page, got full-width text.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16065
Summary:
Ref T7643.
- When a transaction edits a text block, add a link to the changes (for HTML mail).
- Also, inline the changes in the mail (for HTML mail).
- Do nothing for text mail since I don't think we really have room? And I don't know how we can make the diff look any good.
Test Plan:
Edited a task description, generated mail, examined mail.
- It contained a link leading to a prose diff.
- It had a more-or-less reasonable inline text diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16063
Summary:
Ref T10785. Around the time we launched Phacility SAAS we implemented this weird autologin hack. It works fine, so clean it up, get rid of the `instanceof` stuff, and support it for any OAuth2 provider.
(We could conceivably support OAuth1 as well, but no one has expressed an interest in it and I don't think I have any OAuth1 providers configured correctly locally so it would take a little bit to set up and test.)
Test Plan:
- Configured OAuth2 adapters (Facebook) for auto-login.
- Saw no config option on other adapters (LDAP).
- Nuked all options but one, did autologin with Facebook and Phabricator.
- Logged out, got logout screen.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10785
Differential Revision: https://secure.phabricator.com/D16060
Summary:
Ref T10856. The rendering logic was already there, but it was expecting the information under `properties`
field, whereas arc puts it under `metadata`. Not sure if that something that changed a long time ago or if
it was always like this.
Test Plan: {F1252657 size=full}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T10856
Differential Revision: https://secure.phabricator.com/D15828
Summary:
Ref T3353. This hooks the prose engine up to the UI and throws away the hard-wrapping hacks.
These are likely still very rough in many cases, but are hopefully a big step forward from the old version in the vast majority of cases.
Test Plan: {F1677809}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3353
Differential Revision: https://secure.phabricator.com/D16056
Summary:
Fixes T11088. When a task is removed from a project, we don't normally delete its column positions. If you accidentally remove a project and then restore the project, it's nice for the task to stay where you put it.
However, we do need to remove its positions in proxy columns to avoid the issue in T11088.
Test Plan:
- Added a failing unit test, made it pass.
- Added a task to "X > Milestone 1", loaded workboard, used "Edit Projects" to move it to "X" instead, loaded workboard.
- Before, it stayed in the "Milestone 1" column.
- After, it moves to the "Backlog" column.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11088
Differential Revision: https://secure.phabricator.com/D16052
Summary:
Ref T11098.
- Allow "Editor" to be set to the empty string.
- Don't match a validation error to a field unless the actual settings for the field and error match.
Test Plan:
- Tried to set "Editor" to "", success.
- Tried to set "Editor" to "javascript://", only that field got marked "Invalid".
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16051
Summary: Ref T11098. Template preferences don't have a user, but this codepath didn't get fully updated to account for that.
Test Plan: Saved mail tags in global prefernces.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16050
Summary:
Ref T11098. We have a fair number of these, including links in email, which we can't turn into explicit `/user/` URIs.
Just redirect them to the modern places.
Test Plan: Clicked "Customize Menu..." on home page.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16049
Summary:
Ref T4103. This just adds a single global default setting group, not full profiles.
Primarily, I'm not sure how administrators are supposed to set profiles for users, since most ways user accounts get created don't really support setting roles.. When we figure that out, it should be reasonably easy to extend this. There also isn't much of a need for this now, since pretty much everyone just wants to turn off mail.
Test Plan:
- Edited personal settings.
- Edited global settings.
- Edited a bot's settings.
- Tried to edit some other user's settings.
- Saw defaults change appropriately as I edited global and personal settings.
{F1677266}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16048
Summary:
Ref T4103. If the database has `""` (empty string) for select/option settings, we can let that value be effective in the UI right now.
One consequence is that timestamps can vanish from the UI.
Instead, be stricter and discard it as an invalid value.
Test Plan:
- Forced `time-format` setting to `''`.
- Saw timestamps vanish before change.
- Saw timestamps return to the default value after change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16047
Summary:
Ref T4103. Two small improvements:
- Don't work as hard to validate translations. We just need to know if a translation exists, we don't need to count how many strings it has and build the entire menu.
- Allow `getUserSetting()` to work on any setting without doing all the application/visibility checks. It's OK for code to look at, say, your "Conpherence Notifications" setting even if that application is not installed for you.
Test Plan: Used XHProf and saw 404 page drop from ~60ms to ~40ms locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16046
Summary:
Ref T10077. Currently, we issue 6+ queries on every page to build this menu, since the menu is built application-by-application.
Build the menu with dedicated modules instead so a single "EditEngine" module can provide all of them with one query.
I'd like to reduce this to 0 queries but I'm not totally sure what we want to do with this menu.
This change removes these items, because EditEngine can not currently provide them:
- Calendar: Eventually via EditEngine eventually.
- Conpherence: Probably via EditEngine, doesn't seem too important.
- People: Maybe via EditEngine, doesn't seem too important? "Welcome" is likely better?
- Pholio: Eventually via EditEngine.
It adds a bunch of other items as a side effect:
{F1677151}
This reduces the queries issued on every page by ~5.
This also makes quick create actions visible while logged out (see T7073).
Test Plan:
- Viewed menu while logged in.
- Viewed menu while logged out.
- Viewed standalone version of menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10077
Differential Revision: https://secure.phabricator.com/D16045
Summary:
Ref T10078. Currently, you toggle DarkConsole and then load a page, but on the load we have to refill your settings cache since toggling DarkConsole dirtied it.
This is fine, except that it makes it harder to understand what's going on with queries on a page. Just force it to reload right away instead.
Test Plan: Toggled DarkConsole, reloaded page, no longer saw settings toggle-related cache fill.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10078
Differential Revision: https://secure.phabricator.com/D16044
Summary: Ref T4103. Ref T10078. We currently have separate "usable" and "raw" values, but can simplify this by making `newValueForUsers()` return the raw value.
Test Plan: Ran unit tests; browsed around; dropped caches and browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16043
Summary:
Ref T4103. Ref T10078. This puts a user cache in front of notification and message counts.
This reduces the number of queries issued on every page by 4 (2x building the menu, 2x building Quicksand data).
Also fixes some minor issues:
- Daemons could choke on sending mail in the user's translation.
- No-op object updates could fail in the daemons.
- Questionable data access pattern in the file query coming out of the profile file cache.
Test Plan:
- Sent myself notifications. Saw count go up.
- Cleared them by visiting objects and clearing all notifications. Saw count go down.
- Sent myself messages. Saw count go up.
- Cleared them by visiting threads. Saw count go down.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16041
Summary:
Ref T4103. Ref T10078. This moves profile image caches to new usercache infrastructure.
These dirty automatically based on configuration and User properties, so add some stuff to make that happen.
This reduces the number of queries issued on every page by 1.
Test Plan: Browsed around, changed profile image, viewed as self, viewed as another user, verified no more query to pull this information on every page
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16040
Summary:
Ref T4103. Ref T10078. Currently, when a user misses a cache we just build it for them.
This is the behavior we want for the the viewer (so we don't have to build every cache up front if we don't actually need them), but not the right behavior for other users (since it allows performance problems to go undetected).
Make inline cache generation strict by default, then make sure all the things that rely on cache data request the correct data (well, all of the things identified by unit tests, at least: there might be some more stuff I haven't hit yet).
This fixes test failures in D16040, and backports a piece of that change.
Test Plan: Identified and then fixed failures with `arc unit --everything`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16042
Summary: Ref T4103. This method has no more callers.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16039
Summary:
Ref T4103. This isn't completely perfect but should let us move forward without also expanding scope into "too much mail".
I split the existing "Mail Preferences" into two panels: a "Mail Delivery" panel for the EditEngine settings, and a "2000000 dropdowns" panel for the two million dropdowns. This one retains the old code more or less unmodified.
Test Plan:
- Ran unit tests, which cover most of this stuff.
- Grepped for all removed constants.
- Ran migrations, inspected database results.
- Changed settings in both modified panels.
- This covers a lot of ground, but anything I missed will hopefully be fairly obvious.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16038
Summary: Ref T4103. Also get rid of the weird cache clear that nothing else uses and which we don't actually need.
Test Plan:
- Resolved timezone conflict by ignoring it.
- Resolved timezone conflict by picking a valid timezone.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16037
Summary: Fixes T8846. Ref T4103. I just took the shortest reasonable path here, this panel could use some attention on the next Conpherence iteration.
Test Plan: Turned on/off desktop notifications. Observed corresponding behavior in test notifications.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T8846
Differential Revision: https://secure.phabricator.com/D16036
Summary:
Ref T4103. Some settings (mostly nav collapsed/expanded states) use this endpoint to make adjustments when users press keys (like `\` to toggle the durable column).
All of these settings are now formal, so swap things over to transactions.
Test Plan: Collapsed/expanded various navs, reloaded pages, settings stuck.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16035
Summary: Ref T4103. Provide a CLI mechanism for purging the user cache.
Test Plan:
- Purged with `--purge-user` and `--purge-all`.
- Verified cache table got wiped.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16033
Summary: Ref T4103. Fully modernize the filetree show/hide, durable column show/hide, and profile menu collapse/wide settings.
Test Plan:
- Toggled filetree on/off, reloaded page, setting stuck.
- Same with conpherence column and profile menus.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16034
Summary: Ref T4103. This primarily makes sure the console gets turned on when you enable it so you aren't like "where's the console???"
Test Plan: Enabled console, saw console.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16030
Summary:
Ref T4103. These settings long-predate proper settings and are based on hard-coded user properties. Turn them into real settings.
(I didn't try to migrate the value since they're trivial to restore and only useful to developers.)
Test Plan:
- Toggled console on/off.
- Swapped tabs.
- Reloaded page, everything stayed sticky.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16029
Summary:
Ref T4103. A few bits here:
- We have an ancient "tiles" preference which was just a fallback from 2-3 years ago. Throw that away.
- Modenize the other pinned stuff. We should likely revisit this after the next homepage update but I just left the actual defaults alone for now.
- Lightly prepare for global default editing.
- Add a "reset to defaults" option.
Test Plan:
- Pinned, unpinned, reordered and reset application homepage order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16028
Summary:
Ref T4103. Convert this into a proper internal setting and use transactions to mutate it.
Also remove some no-longer-used old non-modular settings constants.
Test Plan:
- Used policy dropdown, saw recently-used projects.
- Selected some new projects, saw them appear.
- Grepped for all removed constants.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16027
Summary: Ref T4103. Modernize the blame/color toggles in Diffusion. These have no separate settings UI.
Test Plan: Toggled blame and colors, reloaded pages, settings stuck.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16026
Summary:
Ref T4103. Conpherence is doing some weird stuff and has its own redudnant settings object.
- Get rid of `ConpherenceSettings`.
- Use `getUserSetting()` instead of `loadPreferences()`.
- When applying transactions, add a new mechanism to efficiently prefill caches (this will still work anyway, but it's slower if we don't bulk-fetch).
Test Plan:
- Changed global Conpherence setting.
- Created a new Conpherence, saw setting set to global default.
- Changed local room setting.
- Submitted messages.
- Saw cache prefill for all particpiants in database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16025
Summary: Ref T4103. This isn't necessary or particularly useful anymore since panels have been converted.
Test Plan: Visited URI, got a 404.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16024
Summary: Ref T4103. This converts other straightforward panels to modern stuff.
Test Plan:
- Edited various settings.
- Tried to set a bogus editor value.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16023
Summary: Ref T4103. Only trick here is hiding the panel if Conpherence is not installed.
Test Plan:
- Edited Conpherence preferences.
- Uninstalled Conpherence, saw panel vanish.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16022
Summary:
Ref T4103. Some settings (like the collapsed/expanded state of the diff filetree) are currently ad-hoc. They weren't being read correctly.
Also, simplify the caching code a little bit.
Test Plan: Toggled filetree, reloaded page, got sticky behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16021
Summary:
Ref T4103. Settings panels are grouped into categories of similar panels (like "Email" or "Sessions and Logs").
Currently, this is done informally, by just grouping and ordering by strings. This won't work well with translations, since it means the ordering is entirely dependent on the language order, so the first settings panel you see might be something irrelvant or confusing. We'd also potentially break third-party stuff by changing strings, but do so in a silent hard-to-detect way.
Provide formal objects and modularize the panel groups completely.
Test Plan: Verified all panels still appear properly and in the same groups and order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16020
Summary:
Ref T4103. We have a couple of settings like this where changing one setting changes another (e.g., enabling DarkConsole makes the console visible).
Provide a mechanism to let changing timezone really mean "change timezone, and also clear the timezone offset".
Test Plan: Swapped timezones, reconciled them by ignoring the offset, changed timezone again to another zone with the same offset, got asked to reconcile again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16018
Summary:
Ref T4103. This pretty much replaces these panels in-place with similar looking ones that go through EditEngine.
This has a few rough edges but they're pretty minor and/or hard to hit (for example, when editing another user's settings, the crumbs have a redundant link in them).
Test Plan:
- Edited my own settings.
- Edited a bot user's settings.
- Tried to edit another user's settings (failed).
{F1674465}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16017
Summary:
Ref T4103. This is just incremental cleanup:
- Add "internal" settings, which aren't editable via the UI. They can still do validation and run through the normal pathway. Move a couple settings to use this.
- Remove `getPreference()` on `PhabricatorUser`, which was a sort of prototype version of `getUserSetting()`.
- Make `getUserSetting()` validate setting values before returning them, to improve robustness if we change allowable values later.
- Add a user setting cache, since reading user settings was getting fairly expensive on Calendar.
- Improve performance of setting validation for timezone setting (don't require building/computing all timezone offsets).
- Since we have the cache anyway, make the timezone override a little more general in its approach.
- Move editor stuff to use `getUserSetting()`.
Test Plan:
- Changed search scopes.
- Reconciled local and server timezone settings by ignoring and changing timezones.
- Changed date/time settings, browsed Calendar, queried date ranges.
- Verified editor links generate properly in Diffusion.
- Browsed around with time/date settings looking at timestamps.
- Grepped for `getPreference()`, nuked all the ones coming off `$user` or `$viewer` that I could find.
- Changed accessiblity to high-contrast colors.
- Ran all unit tests.
- Grepped for removed constants.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16015
Summary:
Ref T4103. This is a weird standalone setting that I didn't clean up earlier.
Also fix an issue with the PronounSetting and the Editor not interacting properly.
Test Plan: Edited using new EditEngine UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16014
Summary: Change metamta.differential.patch-format over to an enum option now that they're implemented.
Test Plan: Looked at settings page.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16032
Summary:
Ref T11092. With Quicksand (or, possibly, some as-yet-unknown non-Quicksand workflow) the client can get stuck with an out-of-date revision PHID.
We then save comments with a `revisionPHID` from one revision and a `changesetID` from a different one.
Detect and prevent this. This stops the workflow immediately when the use first clicks, so it should allow us to detect this issue if it has some other non-Quicksand cause.
Test Plan:
- Opened revision `D123`.
- Pressed `\` to enable the sidebar and Quicksand.
- Clicked a link to revision `D124`.
- Added inlines.
Previously, these could ghost. The exact UI behavior is difficult to describe, but in the database they end up with a `changesetID` for `D124` but the original `revisionPHID` for `D123`, presumably because state is sticking around from the first page.
After this patch, an exception is thrown immediately. Additionally:
- Reloaded to clear quicksand state, added comments fine.
- Disabled sidebar/quicksand, added comments fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11092
Differential Revision: https://secure.phabricator.com/D16031
Summary:
Ref T11076. Ref T9897. Bad links on Phame blogs are currently made worse because we try to prompt you to login on a non-cookie domain.
Instead, just 404 in a vanilla way. Do so cleanly on external domains.
Test Plan: {F1672399}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897, T11076
Differential Revision: https://secure.phabricator.com/D16010
Summary:
Fixes T11082. Currently, the `/123/` and `/CALLSIGN/` versions of the URI get the same score.
Also the scores are backwards.
Test Plan:
- Added `getPublicCloneURI()` output to repository listing.
- Before patch, saw a repository with a callsign list a less-preferred ID-based URI.
- After patch, saw the repository list the more-preferred callsign-based URI.
Reviewers: chad
Reviewed By: chad
Subscribers: nikolay.metchev
Maniphest Tasks: T11082
Differential Revision: https://secure.phabricator.com/D16008
Summary:
Ref T4103. These are currently stored on the user, for historic/performance reasons.
Since I want administrators to be able to set defaults for translations and timezones at a minimum and there's no longer a meaningful performance penalty for moving them off the user record, turn them into real preferences and then nuke the columns.
Test Plan:
- Set settings to unusual values.
- Ran migrations.
- Verified my unusual settings survived.
- Created a new user.
- Edited all settings with old and new UIs.
- Reconciled client/server timezone disagreement.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16005
Summary:
Ref T4103. This doesn't get everything, but takes care of most of the easy stuff.
The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next.
Test Plan:
- Grepped for `loadPreferences` to identify callsites.
- Changed start-of-week setting, loaded Calendar, saw correct start.
- Visited welcome page, read "Adjust Settings" point.
- Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence.
- Enabled Filetree, toggled in Differential.
- Disabled Filetree, no longer visible in Differential.
- Changed "Unified Diffs" preference to "Small Screens" vs "Always".
- Toggled filetree in Diffusion.
- Edited a task, saw sensible projects in policy dropdown.
- Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd.
- Toggled "monospaced textareas", used a comment box, got appropriate fonts.
- Toggled durable column.
- Disabled title glyphs.
- Changed monospaced font to 18px/36px impact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16004
Summary:
Ref T4103. Currently, we issue a `SELECT * FROM user_preferences ... WHERE userPHID = ...` on every page to load the viewer's settings.
There are several other questionable data accesses on every page too, most of which could benefit from improved caching strategies (see T4103#178122).
This query will soon get more expensive, since it may need to load several objects (e.g., the user's settings and their "role profile" settings). Although we could put that data on the User and do both in one query, it's nicer to put it on the Preferences object ("This inherits from profile X") which means we need to do several queries.
Rather than paying a greater price, we can cheat this stuff into the existing query where we load the user's session by providing a user cache table and doing some JOIN magic. This lets us issue one query and try to get cache hits on a bunch of caches cheaply (well, we'll be in trouble at the MySQL JOIN limit of 61 tables, but have some headroom).
For now, just get it working:
- Add the table.
- Try to get user settings "for free" when we load the session.
- If we miss, fill user settings into the cache on-demand.
- We only use this in one place (DarkConsole) for now. I'll use it more widely in the next diff.
Test Plan:
- Loaded page as logged-in user.
- Loaded page as logged-out user.
- Examined session query to see cache joins.
- Changed settings, saw database cache fill.
- Toggled DarkConsole on and off.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16001
Summary:
Ref T4103. This tackles all the easy stuff. Not yet handled:
- Translation, pronoun, timezone: these are weird and stored on the User object instead of in settings.
- Conpherence default: actually just missed this one, it's normal.
- 1000 dropdowns for email notification preferences (messy, technically).
Test Plan:
wow look at all these settings
{F1670442}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D15999
Summary:
Ref T6299. This makes more of the links point to the right places.
Not covered yet:
- Projects and subscribers don't point to the right place (this is a little tricky to fix, I think).
- `[[ #anchor ]]`s won't do the right thing in, uh, email, I guess, since `uri.here` is not set. This is also a little tricky.
Possibly we should just remove subscribers (although also kind of tricky).
Test Plan: On a custom-domain blog, observed that fewer things were broken.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6299
Differential Revision: https://secure.phabricator.com/D16007
Summary: If you try to join or add members to a parent project, we currently return 404. This instead adds an informational dialog. Fixes T11055
Test Plan: Click on Join Project and Add Members while on a Parent Project or Milestone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11055
Differential Revision: https://secure.phabricator.com/D16000
Summary: Ref T4103. Just porting these directly for now, no attempt to organize things yet.
Test Plan: {F1669263}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D15997
Test Plan: click new link, get to the right page
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15996
Summary: Ref T4103. This starts breaking out settings in a modern way to prepare for global defaults.
Test Plan:
- Edited diff settings.
- Saw them take effect in primary settings pane.
- Set stuff to new automatic defaults.
- Tried to edit another user's settings.
- Edited a bot's settings as an administrator.
{F1669077}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D15995
Summary:
Ref T4103. This give preferences a PHID, policy/transaction interfaces, a transaction table, and a Query class.
This doesn't actually change how they're edited, yet.
Test Plan:
- Ran migrations.
- Inspected database for date created, date modified, PHIDs.
- Changed some of my preferences.
- Deleted a user's preferences, verified they reset properly.
- Set some preferences as a new user, got a new row.
- Destroyed a user, verified their preferences were destroyed.
- Sent Conpherence messages.
- Send mail.
- Tried to edit another user's settings.
- Tried to edit a bot's settings as a non-admin.
- Edited a bot's settings as an admin (technically, none of the editable settings are actually stored in the settings table, currently).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D15991
Summary: Seen some complaints about usability here, adding role=dialog to improve when these trigger.
Test Plan: Turn on Voiceover, tab over to log out link, here proper dialog title and text before highlighted submit button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15993
Summary:
Ref T10769. The user availability cache write shouldn't happen in read-only mode, nor should the Differential parse cache write.
(We might want to turn off the availbility feature completely since it's potentially expensive if we can't cache it, but I think we're OK for now.)
Test Plan:
In read-only mode:
- Browsed as a user with an out-of-date availability cache.
- Loaded an older revision without cached parse data.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10769
Differential Revision: https://secure.phabricator.com/D15988
Summary:
Ref T4292. For hosted, clustered repositories we have a good way to increment the internal version of the repository: every time a user pushes something, we increment the version by 1.
We don't have a great way to do this for observed/remote repositories because when we `git fetch` we might get nothing, or we might get some changes, and we can't easily tell //what// changes we got.
For example, if we see that another node is at "version 97", and we do a fetch and see some changes, we don't know if we're in sync with them (i.e., also at "version 97") or ahead of them (at "version 98").
This implements a simple way to version an observed repository:
- Take the head of every branch/tag.
- Look them up.
- Pick the biggest internal ID number.
This will work //except// when branches are deleted, which could cause the version to go backward if the "biggest commit" is the one that was deleted. This should be OK, since it's rare and the effects are minor and the repository will "self-heal" on the next actual push.
Test Plan:
- Created an observed repository.
- Ran `bin/repository update` and observed a sensible version number appear in the version table.
- Pushed to the remote, did another update, saw a sensible update.
- Did an update with no push, saw no effect on version number.
- Toggled repository to hosted, saw the version reset.
- Simulated read traffic to out-of-sync node, saw it do a remote fetch.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15986
Summary:
Ref T11051. This is still not as clear as it should be, but is at least working as intended now.
I believe this part of the code just never worked. The test plan on D10489 didn't specifically cover it.
Test Plan:
Did this sort of thing in a repository:
```
$ git checkout -b featurex
$ echo x >> y
$ git commit -am wip
$ arc diff
```
Then I simulated just pushing it (this flow is a little more involved than necessary):
```
$ arc land --hold
$ git commit --amend
$ # remove all metadata -- particularly, "Differential Revision"!
$ git push HEAD:master
```
I got a not-great but more-useful dialog:
{F1667318}
Prior to this change, the hash match was incorrectly not reported at all.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11051
Differential Revision: https://secure.phabricator.com/D15989
Summary:
Fixes T11042. Currently, all `bin/storage` workflows stop if `cluster.read-only` is set:
```
$ ./bin/storage adjust
Usage Exception: Phabricator is currently in read-only mode. Use --force to override this mode.
```
However, some of them (`status`, `dump`, `databases`, etc) are read-only anyway and safe to run. Don't prompt in these cases.
Test Plan:
- Set `cluster.read-only` to `true`.
- Ran `bin/storage dump`, `bin/storage status`, etc. No longer received messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11042
Differential Revision: https://secure.phabricator.com/D15987
Summary:
Ref T5267. Fixes T9643. Currently, we can not parse/extract a small fraction of strings:
- PHP files which contain obscure syntax that XHPAST can not parse.
- HEREDOCs do not have a useable `evalStatic()` behavior right now.
Emit these as warnings, but continue and generate all usable/extractable translations.
Test Plan:
```
epriestley@orbital ~/dev/phabricator $ ./bin/i18n extract . >/dev/null
Found 4,548 files...
Done.
WARNING: Failed to extract strings from file "/Users/epriestley/dev/core/lib/phabricator/externals/stripe-php/lib/Stripe/ApiRequestor.php": XHPAST Parse Error: syntax error, unexpected T_STRING on line 357
WARNING: Failed to evaluate pht() call on line 141 in "/Users/epriestley/dev/core/lib/phabricator/scripts/repository/commit_hook.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 24 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorClusterConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 23 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 49 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 83 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 91 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 97 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 103 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 113 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 119 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 128 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 137 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 149 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 159 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 166 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 179 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 192 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 24 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorNotificationConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 26 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/harbormaster/step/HarbormasterCircleCIBuildStepImplementation.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 154 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 282 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 84 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/maniphest/editor/ManiphestEditEngine.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 345 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 59 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 26 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/project/config/PhabricatorProjectConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 53 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/project/config/PhabricatorProjectConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 98 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 165 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 252 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 299 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 360 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 439 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 529 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 54 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/controller/PhabricatorEditEngineConfigurationLockController.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 56 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 126 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 185 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 238 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 288 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 17 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/uiexample/examples/PhabricatorRemarkupUIExample.php": Unexpected node during static evaluation, of type: n_HEREDOC
epriestley@orbital ~/dev/phabricator $
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267, T9643
Differential Revision: https://secure.phabricator.com/D15983
Summary:
Fixes T11043. This page was still reading the old information directly instead of going through the cluster-aware stuff.
Have it ask the cluster-aware stuff for information instead.
Test Plan:
- Nuked MySQL on localhost.
- Configured cluster hosts.
- Loaded "Database Status" page -- worked after patch.
- Grepped for any remaining `mysql.configuration-provider` stragglers, came up empty.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11043
Differential Revision: https://secure.phabricator.com/D15982
Summary:
Fixes T8510. Results are internally ordered by "name", which is the full list of strings a user can type to match a result. On the balance, it is probably good/correct to order by this (particularly, it allows `function(x)` to sort near `x`).
However, the way projects were built put the tags first, so a project like "Discovery" could end up last if it had originally been created with a different name like "Search Team", so that its first slug is "search-team".
Instead, put the display name first in the ordering.
Test Plan:
{F1661775}
To reproduce in particular:
- Create a project named "Zebra".
- Create a lot of projects named "Armadillo-blahblahblah".
- Rename "Zebra" to "Armadillo".
Before the patch, the new "Armadillo" project would still sort as though it were named "Zebra". After the patch, it sorts as expected normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8510
Differential Revision: https://secure.phabricator.com/D15981
Summary: Ref T5267. Put "Deutsch" in the list instead of "German", so you can find your language without knowing the English word for it.
Test Plan: {F1661598}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267
Differential Revision: https://secure.phabricator.com/D15980
Summary:
Ref T5267. Ref T4103. Currently, adding new locale support to the upstream fills this menu with confusing options which don't do anything. Separate it into four groups:
- Translations: these have a "reasonable number" of strings and you'll probably see some obvious effect if you switch to the translation.
- Limited Translations: these have very few or no strings, and include locales which we've added but don't ship translations for.
- Silly Translations: Pirate english, etc.
- Test Translations: ALLCAPS, raw strings, etc.
Czech is currently in "test" instead of "limited" for historical reasons; I'll remedy this in the next change.
Test Plan: {F1661523}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T5267
Differential Revision: https://secure.phabricator.com/D15978
Summary:
Ref T4103. This removes these options:
{F1660585}
The jump nav option came from T916, when we had a separate jump nav on the home page. Essentially no one has ever been confused by the behavior of search or disabled this feature. Here are the stats for this install:
| Total Users | 36656 |
| Have Set Any Preference | 3084 |
| Have Disabled Jump | 6
| Are Not "Security Researchers" | 2
| Any Account Activity | 0
The "/" option came in the same change, but the preference came from T989. This keystroke conflicts with a default Firefox keystroke. Almost no one cares about this either, but I count 6 real users who have disabled the behavior. I suspect the number of real users who //use// it may be smaller.
In Safari and Firefox, the "tab" key does the same thing.
In Chrome, the "tab" key does the same thing if {nav Preferences > Web Content > "Pressing Tab highlights..."} is disabled.
Upshot: jump nav is great, bulk of the change in T989 was clearly great, specific preferences that came out of it seem not-so-great and now is a good time to kill them as we head into T4103.
Test Plan:
- Grepped for removed constants.
- Pressed "/".
- Searched for `T123`.
- Viewed settings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D15976
Summary:
Fixes T11030. Fixes T11032.
- Allow HTTP access to "Public" repositories even if `diffusion.allow-http-auth` is disabled.
- If you run Phabricator on an unusual port (???) use that port as the default when generating HTTP URIs.
Test Plan:
- Faked `phabricator.base-uri` to an unusual port, saw repository HTTP URI generate with an unusual port.
- Disabled `diffusion.allow-http-auth`, confirmed that toggling view policy between "public" and "users" activated or deactivated HTTP clone URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11030, T11032
Differential Revision: https://secure.phabricator.com/D15973
Summary:
Ref T11016. I think I inverted the meaning of this function by accident in D14893.
The intent is to return a list of users: direct users, and all members of all projects.
Prior to this patch actually returns direct users, and all projects they are members of.
Test Plan:
- Created "Project with Dog".
- Added user "dog" to project.
- Created package "X", owning file "/x", with audit enabled.
- Made "X" owned by "Project with Dog".
- Modified "/x" and had user "dog" accept it.
- Landed change.
- Prior to change: package "X" incorrectly added as auditor.
- After change: package "X" correctly omitted as auditor, because a member reviewed the change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11016
Differential Revision: https://secure.phabricator.com/D15971
Summary:
Fixes T11017. We add packages as "uninteresting" auditors so that we can query commits by package later.
Until recently, this didn't matter because we didn't send mail to packages. But now we do, so stop mailing them when they don't actually need to do anything.
Test Plan:
- Made a commit to a file which was part of a package but which I owned (so it does not trigger auditing).
- `var_dump()`'d mail "To:" PHIDs.
- Before patch: included package.
- After patch: no package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11017
Differential Revision: https://secure.phabricator.com/D15970
Summary:
Fixes T11020. I think this resolves things -- `$new_version` (set above) should be used, not `$new_log` directly.
Specifically, we would get into trouble if the initial push failed for some reason (working copy not initialized yet, commit hook rejected, etc).
Test Plan: Made a bad push to a new repository. Saw it freeze before the patch and succeed afterwards.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11020
Differential Revision: https://secure.phabricator.com/D15969
Summary: Fixes T11010. This also needs to be inflated until we fix the whole client/server responsibility issue here.
Test Plan:
- Created a revision while observing error log, no error.
- Disabled "allow self accept", tried to make myself a reviewer, got rejected with an error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11010
Differential Revision: https://secure.phabricator.com/D15966
Summary: Ref T9606, Clicking on the calendar preview header in user's profile page should link to user's full month calendar
Test Plan: Open user profile, scroll to calendar preview, click on Calendar box header. This should open the month calendar for the user (not viewer)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9606
Differential Revision: https://secure.phabricator.com/D15967
Summary: Hover hint on calendar list items should be to the right in day view, left in profile view, on top in month view
Test Plan: Open profile view, calendar items should have a left hover. Open day view, calendar items should have a right hover. Open month view, calendar items should have top hover.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9606
Differential Revision: https://secure.phabricator.com/D15964
Summary: Ref T9606
Test Plan: Open people profile for a user with events today/tomorrow, see a panel under badges panel with event list
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9606
Differential Revision: https://secure.phabricator.com/D15851
Summary:
Ref T3025.
- Show current zone to make the current vs new more clear.
- Tweak some text.
Test Plan: {F1656534}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3025
Differential Revision: https://secure.phabricator.com/D15965
Summary:
Fixes T10381. When we converted to `PHUIRemarkupView`, some instructional text got linebreaks added when it shouldn't have them (the source is written in PHP and wrapped at 80 characters, but the output should flow naturally).
Fix this so we don't preserve linebreaks.
This also makes `PHUIRemarkupView` a little more powerful and inches us toward fixing Phame/CORGI remarkup issues, getting rid of `PhabricatorMarkupInterface` / `PhabricatorMarkupOneOff`, and dropping all the application hard-coding in `PhabricatorMarkupEngine`.
Test Plan:
- Grepped for all callsites, looking for callsites which accept remarkup written in `<<<HEREDOC` format.
- Viewed form instructions, Conduit API methods, HTTP parameter edit instructions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10381
Differential Revision: https://secure.phabricator.com/D15963
Summary:
Ref T3025. Chrome gives us an easily-accessible, much better guess at which timezone the user is in.
Firefox also exposes "Intl" but this doesn't seem to be a reliable method to read the timezone.
Test Plan:
In Chrome, swapped my system date/time between zones, clicked the "reconcile" popup, got the dropdown prefilled accurately.
In Safari (no `Intl` API) got the normal flow with no default selected.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3025
Differential Revision: https://secure.phabricator.com/D15962
Summary: Ref T10512. This is fairly bare-bones but appears to work.
Test Plan: Queried all users, queried some stuff by constraints.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10512
Differential Revision: https://secure.phabricator.com/D15959
Summary: Ref T3025. This adds a check for different client/server timezone offsets and gives users an option to fix them or ignore them.
Test Plan:
- Fiddled with timezone in Settings and System Preferences.
- Got appropriate prompts and behavior after simulating various trips to and from exotic locales.
In particular, this slightly tricky case seems to work correctly:
- Travel to NY.
- Ignore discrepancy (you're only there for a couple hours for an important meeting, and returning to SF on a later flight).
- Return to SF for a few days.
- Travel back to NY.
- You should be prompted again, since you left the timezone after you ignored the discrepancy.
{F1654528}
{F1654529}
{F1654530}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3025
Differential Revision: https://secure.phabricator.com/D15961
Summary:
Ref T5187. This definitely feels a bit flimsy and I'm going to hold it until I cut the release since it changes a couple of things about Workflow in general, but it seems to work OK and most of it is fine.
The intent is described in T5187#176236.
In practice, most of that works like I describe, then the `phui-file-upload` behavior gets some weird glue to figure out if the input is part of the form. Not the most elegant system, but I think it'll hold until we come up with many reasons to write a lot more Javascript.
Test Plan:
Used both drag-and-drop and the upload dialog to upload files in Safari, Firefox and Chrome.
{F1653716}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5187
Differential Revision: https://secure.phabricator.com/D15953
Summary: Makes the background transparent for uploaded thumbs. This page in general needs lots of work, but here's the minimum. Fixes T10986
Test Plan: Edit a Mock with a transparent jeff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10986
Differential Revision: https://secure.phabricator.com/D15957
Summary: Couple of edge cases here I never cleaned up. This inlines points and projects better, with spacing and use of grey to better differentate from project tag colors.
Test Plan:
Review edge cases on workboard with multiple short and long project names.
{F1653998}
{F1653999}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15956
Summary: Converts to table so text wraps on long strings well, button always stays top right, better spacing underneath.
Test Plan: Mail, Gmail, mobile
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15955
Summary:
Ref T10939. This makes the `viewer()` function work again. It retains its own meaning (viewer, plus all their projects and packages).
There's no `exact-viewer()` function; we could conceivably add one eventually if we need it.
Test Plan:
- Queried for `viewer()`, got the same results as querying by my own username.
- Browsed function in token browser.
- Reviewed autogenerated documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15951
Summary:
Ref T10939. If you haven't installed a dashboard, we show an "Active Revisions" panel on the homepage by default. I waited a bit to update this, but the new buckets don't seem to have caused any major problems so far.
Update this to use the new logic. I'm just showing "must review" + "should review", which is similar to the old beahvior.
Also replace the notification count with this same number. This is a little different from the old behavior, but simpler, and I think we should probably move toward getting rid of these counts completely.
Test Plan:
- Viewed homepage as logged-in user, saw my revisions (including revisions I have authority over only because of project membership).
- Saw consistent notification count.
- Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15950
Summary: Ref T10917. This is getting added as a link right now, which causes it to get `<a href>`'d in HTML mail. Add it as text instead.
Test Plan: Edited a key, examined HTML mail body carefully.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15952
Summary:
Ref T10917. This cheats fairly heavily to generate SSH key mail:
- Generate normal transaction mail.
- Force it to go to the user.
- Use `setForceDelivery()` to force it to actually be delivered.
- Add some warning language to the mail body.
This doesn't move us much closer to Glorious Infrastructure for this whole class of events, but should do what it needs to for now and doesn't really require anything sketchy.
Test Plan: Created and edited SSH keys, got security notice mail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15948
Summary:
Ref T10917. Converts web UI edits to transactions.
This is about 95% "the right way", and then I cheated on the last 5% instead of building a real EditEngine. We don't need it for anything else right now and some of the dialog workflows here are a little weird so I'm just planning to skip it for the moment unless it ends up being easier to do after the next phase (mail notifications) or something like that.
Test Plan: {F1652160}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15947
Summary:
Ref T10917. This primarily prepares these for transactions by giving us a place to:
- review old deactivated keys; and
- review changes to keys.
Future changes will add transactions and a timeline so key changes are recorded exhaustively and can be more easily audited.
Test Plan:
{F1652089}
{F1652090}
{F1652091}
{F1652092}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15946
Summary:
Via HackerOne. Currently, you can use "Lock Permanently" to lock a credential permanently, but you can still enable Conduit API access to it. This directly contradicts both intent of the setting and its description as presented to the user.
Instead:
- When a credential is locked, revoke Conduit API access.
- Prevent API access from being enabled for locked credentials.
- Prevent API access to locked credentials, period.
Test Plan:
- Created a credential.
- Enabled API access.
- Locked credential.
- Saw API access become disabled.
- Tried to enable API access; was rebuffed.
- Queried credential via API, wasn't granted access.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15944
Summary:
Ref T10917. Currently, when you delete an SSH key, we really truly delete it forever.
This isn't very consistent with other applications, but we built this stuff a long time ago before we were as rigorous about retaining data and making it auditable.
In partiular, destroying data isn't good for auditing after security issues, since it means we can't show you logs of any changes an attacker might have made to your keys.
To prepare to improve this, stop destoying data. This will allow later changes to become transaction-oriented and show normal transaction logs.
The tricky part here is that we have a `UNIQUE KEY` on the public key part of the key.
Instead, I changed this to `UNIQUE (key, isActive)`, where `isActive` is a nullable boolean column. This works because MySQL does not enforce "unique" if part of the key is `NULL`.
So you can't have two rows with `("A", 1)`, but you can have as many rows as you want with `("A", null)`. This lets us keep the "each key may only be active for one user/object" rule without requiring us to delete any data.
Test Plan:
- Ran schema changes.
- Viewed public keys.
- Tried to add a duplicate key, got rejected (already associated with another object).
- Deleted SSH key.
- Verified that the key was no longer actually deleted from the database, just marked inactive (in future changes, I'll update the UI to be more clear about this).
- Uploaded a new copy of the same public key, worked fine (no duplicate key rejection).
- Tried to upload yet another copy, got rejected.
- Generated a new keypair.
- Tried to upload a duplicate to an Almanac device, got rejected.
- Generated a new pair for a device.
- Trusted a device key.
- Untrusted a device key.
- "Deleted" a device key.
- Tried to trust a deleted device key, got "inactive" message.
- Ran `bin/ssh-auth`, got good output with unique keys.
- Ran `cat ~/.ssh/id_rsa.pub | ./bin/ssh-auth-key`, got good output with one key.
- Used `auth.querypublickeys` Conduit method to query keys, got good active keys.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15943
Summary: Ref T10694. Switch default mode to HTML since it has a number of significant advantages and we haven't seen reports of significant problems.
Test Plan:
- Switched preference to default (saw "HTML" in UI).
- Sent myself some mail.
- Got HTML mail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15885
Summary:
Ref T10694. If this feels good, I'd plan to eventually add something similar to other applications ("View Task", etc).
Not sure if we should keep the object link later in the mail body or not. I left it for now.
Test Plan: {F1307256, size=full}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15884
Summary: Wow! Real value here.
Test Plan: No more red underlines.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15941
Summary: Fixes T10493. See that task and inline comments for discussion.
Test Plan:
Created an object with some projects, saw the transaction in resulting mail:
{F1600496}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10493
Differential Revision: https://secure.phabricator.com/D15942
Fixes T10981. Ref T10939. `arc` currently has some odd, hard-coded checks
(missing reviewers, all reviewers away) that depend on the field value being
in a certain format.
The recent changes swapped the field value from scalars (PHIDs) to
dictionaries and broke this workflow. It worked fine in testing because we
apply these checks very inconsistently (not on update or `--edit`).
To get around this for now, serialize into "PHID!" and then unserialize on
the other side. This is icky but keeps us from needing to require an `arc`
upgrade.
These checks are generally bad news and should move to the server side in the
long run (T4631).
(This probably prevents clean `arc diff`, so I'm just cowboy committing it.)
Auditors: chad
Summary:
Ref T10939. Fixes T10174. We can currently trigger "uninteresting" auditors in two ways:
- Packages with auditing disabled ("NONE" audits).
- Packages with auditing enabled, but they don't need an audit (e.g., author is a pacakge owner; "NOT REQUIRED" audits).
These audits aren't interesting (we only write them so we can list "commits in this package" from other UIs) but right now they take up the audit slot. In particular:
- They show in the UI, but are generally useless/confusing nowadays. The actual table of contents does a better job of just showing "which packages do these paths belong to" now, and shows all packages for each path.
- They block Herald from adding real auditors.
Change this:
- Don't show uninteresting auditors.
- Let Herald upgrade uninteresting auditors into real auditors.
Test Plan:
- Ran `bin/repository reparse --owners <commit> --force`, and `--herald` to trigger Owners and Herald rules.
- With a package with auditing disabled, triggered a "None" audit and saw it no longer appear in the UI with the patch applied.
- With a package with auditing disabled, added a Herald rule to trigger an audit. With the patch, saw it go through and upgrade the audit to "Audit Required".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10174, T10939
Differential Revision: https://secure.phabricator.com/D15940
Summary:
Ref T10939. Fixes T10181. This slightly simplifies, then documents the auditing rules, which haven't been updated for a while. In particular:
- If an owner authored the change, never audit.
- Examine all reviewers to determine reviewer audit status, not just the first reviewer.
- Simplify some of the loading code a bit.
Test Plan:
- Ran `bin/repository reparse --owners <commit> --force` to trigger this stuff.
- Verified that the web UI did reasonable things with resulting audits.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10181, T10939
Differential Revision: https://secure.phabricator.com/D15939
Summary: Ref T10939. This adds UI, transactions, etc, to adjust dominion rules.
Test Plan:
- Read documentation.
- Changed dominion rules.
- Created packages on `/` ("A") and `/x` ("B") with "Auto Review: Review".
- Touched `/x`.
- Verified that A and B were added with strong dominion.
- Verified that only B was added when A was set to weak dominion.
- Viewed file in Diffusion, saw correct ownership with strong/weak dominion rules.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15936
Summary:
Ref T10939. This supports two settings for packages (although they can't be configured yet):
- **Strong Dominion**: If the package owns `a/`, it always owns every subpath, even if another package also owns the subpath. For example, if I own `src/differential/`, I always own it even if someone else claims `src/differential/js/` as part of the "Javascript" package. This is the current behavior, and the default.
- **Weak Dominion**: If the package owns `a/`, but another package owns `a/b/`, the package gives up control of those paths and no longer owns paths in `a/b/`. This is a new behavior which can make defining some types of packages easier.
In the next change, I'll allow users to switch these modes and document what they mean.
Test Plan:
- Ran existing unit tests.
- Added new unit tests.
Reviewers: chad
Reviewed By: chad
Subscribers: joel
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15935
Summary: Ref T10939. Fixes T4887. Supports "username!" to add a reviewer as blocking.
Test Plan: Added and removed blocking and non-blocking reviewers via CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4887, T10939
Differential Revision: https://secure.phabricator.com/D15934
Summary:
Ref T10939. Adds a `blocking(...)` token.
This code is pretty iffy and going to get worse before it gets better, but the fix (T10967 + EditEngine) is going to be a fair chunk of work down the road.
Test Plan: {F1426966}
Reviewers: chad
Reviewed By: chad
Subscribers: scode
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15933
Summary: Fixes T10975. The "scramble attached file permissions when an object is saved" code is misfiring here too. See T10778 + D15803 for prior work.
Test Plan:
- Ran `bin/storage upgrade -f`.
- Edited the view policy of an OAuth server (prepatch: fatal; postpatch: worked great).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10975
Differential Revision: https://secure.phabricator.com/D15938
Summary: Fixes T10972. Nothing actually updates this anymore, and only repositories ever did (e.g., Harbormaster and Drydock have never tracked it). Keeping track of this is more trouble than it's worth.
Test Plan: Grepped for constants, viewed a passphrase credential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10972
Differential Revision: https://secure.phabricator.com/D15932
Summary: The S3 fields are mandatory and if you only enter the ones in the docs you will immediately encounter a "Amazon S3 is Only Partially Configured" error. For Mailgun the error is more difficult to figure out - emails get stuck on Mailgun's side but the error is illegible. On Phabricator's side you have to go trawling through nginx logs to find "Mail signature is not valid. Check your Mailgun API key."
Test Plan: Deploy a new standalone instance, follow old instructions, fail. Deploy another one, follow updated instructions, win.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15931
Summary:
Ref T10939. I don't think this is hugely important, but it doesn't clutter things up much and it's nice as a hint.
T4055 was the original request specifically asking for this. It wanted a separate bucket, but I think this use case isn't common/strong enough to justify that.
I would like to improve Differential's "X depends on Y" feature in the long term. We don't tend to use/need it much, but it could easily do a better and more automatic job of supporting review of a group of revisions.
Test Plan: {F1426636}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15930
Summary: Ref T10939. These poor stragglers got left out in the rain. Didn't catch any issues otherwise.
Test Plan: {F1426604}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15929
Summary: Ref T10939. For various historical reasons, revision status is a numeric string. This comparison fails because it's `(string) !== (int)`. Just use `!=` so this will still work if we turn it into a real string in the future.
Test Plan: Tried a more specific test case locally, got better looking results in "Must Review" and "Should Review".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15928
Summary:
Ref T10939. I'm not //totally// opposed to the existence of this element, but I think it's the kind of thing that would never make it upstream today. I think this should just be a T418 custom sort of thing in the long run, not a mainline upstream feature.
Overall, I think this thing is nearly useless and just adds visual clutter. My dashboard is about 100% red. This also sort of teaches users that it's fine to let revisions sit for a couple of days, which isn't what I'd like the UI to teach. Finally, removing it helps the UI feel a little less cluttered after the visually busy changes in D15926.
Test Plan: Grepped for removed config. Viewed revision list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15927
Summary:
Ref T10939. Fixes T9263. Ref T4144.
First, this resolves users (converting users into all packages and projects they are responsible for) earlier, so bucketing can act on that data correctly. Previously, your own blocking reviews would appear in "Must Review" but your packages/projects' would not. Now, all of them will.
Second, this adds `exact(username)` to mean "just me, not my packages/projects". You can use this along with "Bucket: By Required Action" to create a personal view of "Active Revisions" if you'd like, and ignore all your project/package reviews.
Test Plan: Queried by "me" and "exact(me)", got reasonable looking results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T9263, T10939
Differential Revision: https://secure.phabricator.com/D15925
Summary:
Ref T10939. Ref T4144. This splits the existing buckets ("Blocking Others", "Action Required", "Waiting on Others") into 6-7 buckets with a stronger focus on what the next action you need to take is.
See T10939#175423 for some discussion.
Overall, I think some of the root problems here are caused by reviewer laziness and shotgun review workflows (where a ton of people get automatically added to everything, probably unnecessarily), but these buckets haven't been updated since the introduction of blocking reviewers or project/package reviewers and I think splitting the 3 buckets into 6 buckets isn't unreasonable, even though it's kind of a lot of buckets and the root problem here is approximately "I want to ignore a bunch of stuff on my dashboard".
I didn't remove the old bucketing code yet since it's still in use on the default homepage.
This also isn't quite right until I fix the tokenizer to work properly, since it won't bucket project/package reviewers accurately.
Test Plan: {F1395972}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T10939
Differential Revision: https://secure.phabricator.com/D15924
Summary:
Ref T10939. Currently, Differential hard-codes some behaviors for the "active" filter. This introduces "buckets" to make this grouping behavior more general/flexible.
The buckets don't actually do any grouping yet, this just gets rid of the `$query === 'active'` stuff so far.
These buckets change the page size to a large value, becuase pagination won't currently work with bucketing.
The problem is that we normally paginate by selecting one more result than we need: so if we're building a page of size 10, we'll select 11 results. This is fast, and if we get 11 back, we know there's a next page with at least one result on it.
With buckets, we can't do this, since our 11 results might come back in these buckets:
- A, B, C, A, C, C, A, A, B, B, (B)
So we know there are more results, and we know that bucket B has more results, but we have no clue if bucket A and bucket C have more results or not (or if there's anything in bucket D, etc).
We might need to select a thousand more results to get the first (D) or the next (A).
So we could render something like "Some buckets have more results, click here to go to the next page", but users would normally expect to be able to see "This specific bucket, A, has more results.", and we can't do that without a lot more work.
It doesn't really matter for revisions, because almost no one has 1K of them, but this may need to be resolved eventually.
(I have some OK-ish ideas for resolving it but nothing I'm particularly happy with.)
Test Plan: {F1376542}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15923
Summary:
Ref T10939. Ref T4144. This moves the revision SearchEngine to modern code so I can add some kind of bucketing layer on top of it.
This seems to have worked pretty cleanly. One thing is that I removed the ability to search for "pending drafts":
- This was added in D1927 from a bootcamp task, was an indirect solution to a questionable problem, and almost certainly would not meet the bar today.
- Later, in D3324, we added the icons to the list. I think this is a better solution in general. In particular, it specifically addressed the query being kind of junky.
- At the time, Differential had a prebuilt "Drafts" filter. This was removed in D6347 with the move to ApplicationSearch, which simplified the large number of prebuilt filters. Although we got a lot of feedback about that, none requested that the drafts filter be restored.
Test Plan: Searched for responsible users, subscribers, orders, projects, repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T10939
Differential Revision: https://secure.phabricator.com/D15921
Summary:
Looks like some copy pasta snuck in. Also fixes a missed
parenthesis.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15920
Summary:
Ref T10939. If you already own a package, don't trigger the subscribe/review rules.
Document how these rules work.
Test Plan:
- Read documentation.
- Removed reviewers, updated a revision, got autoreviewed.
- Joined package.
- Removed reveiwers, updated a revision, no more autoreview.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15918
Summary: Ref T10939. Packages are valid reviewers, so let Herald "Add Reviewers" and "Add Blocking Reviewers" actions add them.
Test Plan:
- Wrote a rule to add package reviewers.
- Hit the rule, saw a package reviewer added, viewed transcript.
{F1311731}
{F1311732}
{F1311733}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15917
Summary:
Ref T10939. Fixes T8887. This enables and implements the "review" and "blocking review" options for packages.
This is a bit copy-pastey from `DifferentialReviewersHeraldAction`, which doesn't feel awesome. I think the right fix is Glorious Infrasturcture, though -- I filed T10967 to track that.
Test Plan:
- Set package autoreveiw to "Review".
- Updated, got a reveiwer.
- Set autoreview to "blocking".
- Updated, got a blocking reviewer.
{F1311720}
{F1311721}
{F1311722}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15916
Summary:
Ref T10939. Ref T8887. This moves toward letting packages automatically become reviewers or blocking reviewers of owned code.
This change adds an "Auto Review" option to packages. Because adding reviewers/blocking reviewers is a little tricky, it doesn't actually have these options yet -- just a "subscribe" option. I'll do the reviewer work in the next update.
Test Plan:
Created a revision in a package with "Auto Review: Subscribe to Changes". The package got subscribed.
{F1311677}
{F1311678}
{F1311679}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15915
Summary:
Ref T10939. This lets you add packages as reviewers manually.
"Project Reviewers" now lists both projects and packages. I have renamed this to "Coalition Reviewers" but that's probably horrible and confusing. I'm not sure "Group Reviewers" is much better.
Test Plan:
- Added a package as a reviewer manually.
- Joined it, got authority over it.
- Saw the review on my dashboard.
- Accepted the revision, got authority extended to the package review.
{F1311652}
{F1311653}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15914
Summary:
Ref T10939. These appear in "Subscribers" tokenizers now and we got a maybe slightly better icon in the last FA update: {icon shopping-bag} instead of {icon list-alt}.
(I don't feel strongly about this, the old icon just doesn't seem very evocative.)
Test Plan:
o.( O___O ).o
{F1311641}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15913
Summary:
Ref T10939. Fixes T7834.
- Make packages into mailable objects, like projects and users.
- Packages resolve recipients by resolving project and user owners into recipients.
Test Plan:
- Added a comment to a revision with a package subscriber.
- Used `bin/mail show-outbound` to see that owners got mail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7834, T10939
Differential Revision: https://secure.phabricator.com/D15912
Summary:
Ref T10939. This allows the CLI to parse reviewers and subscribers like this:
```Reviewers: epriestley, O123 Some Package Name```
The rule goes:
- If a reviewer or subscriber starts with a monogram (like `X111`), just look that up and ignore everything until the next comma.
- Otherwise, split it on spaces and look up each part.
This means that these are valid:
```
alincoln htaft
alincoln, htaft
#a #b epriestley
O123 Some Package, epriestley, #b
```
I think the only real downside is that this:
```
O123 Some Package epriestley
```
...ignores the "epriestley" part. However, I don't expect users to be typing package monograms manually -- they just need to be representable by `arc land` and `arc diff --edit` and such. Those flows will always add commas and make the parse unambiguous.
Test Plan:
- Added test coverage.
- `amend --show`'d a revision with a package subscriber (this isn't currently possible to produce using the web UI, it came from a future change) and saw `Subscribers: O123 package name, usera, userb`.
- Updated a revision with a package subscriber.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15911
Summary:
Ref T10939. This isn't ideal because it's easy to confuse with zero ("O" vs "0") but I think this will mostly be read-only so it's probably one of the least-bad uses we could make of "O". We haven't really gotten into trouble with "I" (vs "1") for initiatives. Still, open to better ideas.
The goal here is to allow commit messages to include packages in some reasonable way, like `Reviewers: O123 Package Name, epriestley, alincoln`. The parser will ignore the "Package Name" part, that's just for humans. And I don't expect humans to type this, but when the use `arc diff --edit` or similar to update an //existing// revision, the reviewer needs to be represented somehow. It also needs to appear in the commit messages that `arc land` finalizes somehow.
I didn't hook up `/O123` as a URI, but this should do everything else I think.
Test Plan:
- Viewed package list.
- Viewed package detail.
- Did global search for `O12`.
- Used `O12` and `{O12}` remarkup rules.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15910
Summary:
Ref T10939. This has no effect yet since packages can not actually become reviewers, I'm just inching toward support.
- When searching for "responsible users", include revisions that need review by packages you have authority over.
- When calculating review authority, include authority over packages you are a member of (these currently never exist).
Test Plan:
This isn't reachable so I just `var_dump()`'d stuff and looked at the generated queries, which appeared correct/reasonable.
I'll vet this more thoroughly once packages can actually become reviewers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15909
Summary: Ah, a fine thing it be.
Test Plan: Pip pip cheerio.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15919
Summary:
Ref T10923. This extension needs to load a little more data (with `needURIs`) to function correctly now.
(There's a recent migration does this, so indexes got updated correctly when it ran, so it hasn't been obvious that they weren't getting updated properly after that.)
Test Plan: Made an arbitrary edit to a repository, observed no more error in daemon logs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15908
Summary:
Ref T10939. This is just a bug. I thought this was what was described in T10174 but that's actually talking about something completely different.
Also make a `<select />` slightly easier to use.
Test Plan:
- Created a package with auditing enabled.
- Pushed a change.
- Saw audit trigger.
- Disabled the package, pushed a change.
- Before patch: saw audit trigger improperly.
- After patch: restarted daemons, then saw audit correctly not trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15907
Summary:
Ref T4292. Currently, we hold one big lock around the whole `bin/repository update` workflow.
When running multiple daemons on different hosts, this lock can end up being contentious. In particular, we'll hold it during `git fetch` on every host globally, even though it's only useful to hold it locally per-device (that is, it's fine/good/expected if `repo001` and `repo002` happen to be fetching from a repository they are observing at the same time).
Instead, split it into two locks:
- One lock is scoped to the current device, and held during pull (usually `git fetch`). This just keeps multiple daemons accidentally running on the same host from making a mess when trying to initialize or update a working copy.
- One lock is scoped globally, and held during discovery. This makes sure daemons on different hosts don't step on each other when updating the database.
If we fail to acquire either lock, assume some other process is legitimately doing the work and bail more quietly instead of fataling. In approximately 100% of cases where users have hit this lock contention, that was the case: some other daemon was running somewhere doing the work and the error didn't actually represent an issue.
If there's an actual problem, we still raise a diagnostically useful message if you run `bin/repository update` manually, so there are still tools to figure out that something is hung or whatever.
Test Plan:
- Ran `bin/repository update`, `pull`, `discover`.
- Added `sleep(5)`, forced processes to contend, got lock exceptions and graceful exit with diagnostic message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15903
Summary: Ref T10959. This does not fix the problem because the `.differential-diff td` rule is still stronger, but it does let you choose a more compact or breezy style for remarkup blocks and pastes.
Test Plan:
- Set font to `24px / 48px impact`.
- Viewed a paste, saw lovely readable text.
- Viewed an inline code block which was very easy on the eyes.
{F1310420}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10959
Differential Revision: https://secure.phabricator.com/D15904
Summary:
Fixes T10940. Two issues currently:
First, `PullLocal` deamon refuses to update non-cluster repositories on cluster devices. However, this is surprising/confusing/bad because as soon as you enroll a repository host in the cluster, most of the repositories on it stop working until you `clusterize` them. This is especially confusing because the documentation gives you a very nice, gradual walkthrough about going through things slowly and being able to check your work at every step, but we really drop you off a bit of a cliff here. The workflow implied by the documentation is a desirable one.
This operation is generally only unsafe/problematic if the daemon would be creating a //new// working copy. If a working copy already exists, we can reasonably guess that it's almost certainly because you've enrolled a previously un-clustered host into a new cluster. This allows the nice, gradual workflow the documentation describes to proceed as expected, without any weird surprises.
Instead of refusing to update these repositories, only refuse to update them if updating would create a new working copy. This should make transitioning much smoother without any meaningful reduction in safety.
Second, the lower-level `bin/repository update`, `refs`, `mirror`, etc., commands don't apply this same check. However, these commands are potentially just as dangerous. Use the same code to do a similar check there, making sure we only operate on repositories that are either expected to be on the current device, or which already exist here.
Test Plan:
- Ran `bin/phd debug pull`, saw diagnostic information choose to update most repositories (including some non-cluster repositories) but properly skip non-cluster repositories that do not exist locally.
- Ran `bin/repository update`, etc., saw the command apply consistent rules to the rules applied by `PullLocal` and refuse to update non-local repositories it would need to create.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10940
Differential Revision: https://secure.phabricator.com/D15902
Summary: Fixes T9790. This uses a simple renderer, like the inline context renderer, that emphasizes getting a quick glance at small changes and working reasonably on mobile devices.
Test Plan:
- Set `inline` setting to `9999`.
- Created a diff.
- Saw it render reasonably in HTML mail.
- Also tested text mail to make sure I didn't break that.
{F1310137, size=full}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15901
Summary:
Ref T10923. Currently, users can disable or enable builtin URIs, but this doesn't actually do anything.
The behavior of "disable" has changed a bit over time and might need some further refinement, but it's currently meaningless for builtin URIs. Prevent adjustment of it. If users want to hide a URI, they should set "Display: Hidden" instead.
Test Plan:
- Disabled/enabled a non-builtin URI.
- Tried to disable a builtin URI, saw greyed out UI and got a helpful error message.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15899
Summary: Ref T10751. I think this mostly brings us up to date with the state of the world.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15897
Summary: Ref T10923. Fixes T10955. This was accidentally excluded when I broke the form into pages.
Test Plan: Saw edit field in panel; changed project tags for a repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923, T10955
Differential Revision: https://secure.phabricator.com/D15896
Summary: Ref T10751. These workflows have separate `getUser()` and `getViewer()` for weird legacy reasons. `getUser()` is correct.
Test Plan:
- Did a Git SSH push, verified that "Last Writer" reflected the proper user in the "Storage" UI in repository management.
- Grepped for other callsites, double-checked that they used correct users.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15893
Summary: Fixes T10952. Fixes T10930. I didn't implement this method correctly when I expanded this field for repositories.
Test Plan: Edited a paste without warnings.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T10930, T10952
Differential Revision: https://secure.phabricator.com/D15892
Summary:
Ref T10923. This makes the "Clone URI" UI a little nicer:
- Show whether each URI is read-only, read-write, or external.
- Clicking the button selects the URI.
- Add a link to manage the appropriate credentials.
Test Plan: {F1308302, size=full}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15891
Summary: Ref T10923. This links up some more related documentation about repository management.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15890
Summary: These parameters wrongly extend List.
Test Plan:
Used createdStart field for a search - didn't get error about "should be a list".
`git grep 'extends ConduitListParameterType'`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15881
Summary:
Ref T10923. We sort of dead-end new users creating repositories right now, by dumping them into the manage UI without an obvious way forward.
You can click the crumb to get to the repository, but by default it will say something like `R1` which isn't very obvious.
Add a more obvious navigational link to get to the main view.
Test Plan: {F1308196}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15889
Summary:
Ref T10923. The old behavior was to show a full command in SVN, Mercurial, and Git, like this:
- `git clone <uri>`
- `hg clone <uri>`
- `svn checkout <uri> <directory>`
In Git and Mercurial, the `<uri>` ends in something like `/nice-repository-name.git` so the default directory it creates is called `nice-repository-name/`.
In Subversion, we don't (and can't easily) do that for various reasons so we provide an explicit `<directory>` with the nice name.
In the update, I've changed things to just show the URI. I often found that I wanted the URI alone, not the whole clone command (for example, to `fetch`, `remote-add`, etc). This is also consistent with GitHub. Because we have nice URIs for Git and Mercurial, `git clone <uri>` has good behavior.
In Subversion, `svn checkout <uri>` has bad beahvior (you get a directory named `47/` or whatever). So continue showing the whole command there.
We can possibly tailor this after T4245 finishes up and we get access to `/source/nice-repository-name/` URIs.
Test Plan:
- Viewed a Subversion repository, saw a full command.
- Viewed a Git repository, saw only a clone URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15888
Summary: Fixes T10948. Ref T10923. Make these rules a little more thorough and document their behavior.
Test Plan: Looked at Diffusion clone URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923, T10948
Differential Revision: https://secure.phabricator.com/D15887
Summary: Fixes T10941. This avoids a confusing dead end when configuring Subversion hosting, where `svnserve` will fail to execute hooks if the CWD isn't readable by the vcs-user.
Test Plan:
- Updated and committed in a hosted SVN repository.
- Ran some git operations, too.
- @dpotter confirmed this locally in T10941.
Reviewers: chad
Reviewed By: chad
Subscribers: dpotter
Maniphest Tasks: T10941
Differential Revision: https://secure.phabricator.com/D15879
Summary: Ref T10694. This setting no longer has any effect: we always show a limited amount of context now.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15886
Summary: Ref T10923. Although I'd ideally like to get rid of this eventually, keep it around for now.
Test Plan:
- Edited value for an SVN repository.
- Observed no panel present for a Git repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15883
Summary: Fixes T10815. We already recovered reasonably from this for cluster repositories, but not for non-cluster repositories.
Test Plan:
- Viewed cluster and non-cluster empty Git repository.
- Viewed cluster and non-cluster empty Mercurial repository.
- Viewed cluster and non-clsuter empty hosted SVN repository.
- Viewed cluster and non-cluster empty observed SVN repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10815
Differential Revision: https://secure.phabricator.com/D15878
Summary:
Ref T10923. Fixes T9554.
When hosting a repository, we currently have a heuristic that tries to detect when you're doing an initial import: if you push more than 7 commits to an empty repository, it counts as an import and we disable mail/feed/etc.
Do something similar for observed repositories: if the repository is empty and we discover more than 7 commits, switch to import mode until we catch up.
This should align behavior with user expectation more often when juggling hosted vs imported repositories.
Test Plan:
- Created a new hosted repository.
- Activated it and allowed it to fully import.
- Added an "Observe URI".
- Saw it automatically drop into "Importing" mode until the import completed.
- Swapped it back to hosted mode.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9554, T10923
Differential Revision: https://secure.phabricator.com/D15877
Summary:
Ref T10923. When regenerating the URI index for a repository, index every URI.
- Also, make the index slightly stricter (domain + path instead of just path). Excluding the domain made more sense when we were generating only first-party URIs.
- Make the index smarter about `/diffusion/123/` URIs.
- Show normalized URIs in `diffusion.repository.search` results.
Test Plan:
- Ran migration.
- Verified sensible-looking results in database.
- Searched for a repository URI by first-party clone URI.
- Searched for a repository URI by mirror URI.
- Used `diffusion.repository.search` to get information about repository URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15876
Summary: Ref T10923. Walk users through the "create, configure, activate" workflow a little better and set expectations more clearly.
Test Plan:
- Created a new repository, saw new UI help.
- Activated repository, saw onboarding help disappear.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15875
Summary:
Ref T10923.
- Hide "Automation", "Staging" and "Branches" in repositories where they do nothing.
- Fix SVN SSH URIs to read "svn+ssh://" and have proper paths.
Test Plan:
- Verified irrelevant sections did not appear in Subversion in Manage UI.
- Checked out a new hosted SVN repository.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15874
Summary:
Ref T10923. Fixes T10406. This brings most of the guidance/instructions forward:
- Some remained as instructions.
- Some moved to documentation.
Test Plan: Went through all of the sections and hit the help.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10406, T10923
Differential Revision: https://secure.phabricator.com/D15873
Summary: Ref T10923. This provides a little guidance about hosted vs observed, and points at the `diffusion.ssh-*` options.
Test Plan: Poked around in the web UI, saw useful guidance.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15872
Summary: Ref T10923. This cleans up the remaining "pro" mess left by the cutover.
Test Plan: Viewed, managed, edited a repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15871
Summary:
Ref T10923. Primarily documents the process for creating repositories via the API.
Also fixes a couple of issues with `repositoryPHID` not being set yet when creating URIs via the API.
Test Plan:
- Followed all documented steps to create a new repository.
- Created and edited some new URIs from the web workflow, too.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15870
Summary: Ref T10923. This isn't complete yet, but reduces lies and increases truths.
Test Plan: Read documentation, clicked new "Documentation" nav item.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15868
Summary:
Ref T10923. Some of the dialogs ("Deactivate Repository", "Test Automation", etc.) had cancel or redirect URIs which I missed originally.
Go through them and make sure they all point to the right places.
Also removed one unused controller which I missed the first time around.
Test Plan:
- Opened all these dialogs in a new tab with Command-Click.
- Clicked every "cancel" and "submit" button on all of these dialogs.
- Got consistently sent to the place I came from.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15867
Summary:
Ref T10923.
- The "Policy" edit form currently goes "Push, View, Edit". Reorder the defaults to "View, Edit, Push".
- Editing Spaces doesn't currently work: the element appears in the UI, but isn't actually processed when handling transactions. Make that work.
Test Plan:
- Edited a repository policies, saw "View, Edit, Push".
- Moved a repository between Spaces.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15866
Summary:
Ref T10923. Paging wasn't being applied correctly when creating //new// repositories after API changes.
Also, the first redirect after creation wasn't sending users to the right place.
Test Plan:
- Created a new repostiory, got redirected properly.
- Verified that new repostiory flow has the correct fields (name, callsign, etc) and Conduit API has the correct fields (everything).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15865
Summary:
Ref T10694. General improvements:
- Remove leading empty lines from context snippets.
- Remove trailing empty lines from context snippets.
- If we removed everything, render a note.
- Try using `style` instead of `<pre>`? My thinking is that maybe Airmail has weird default rules for `<pre>`, since that's the biggest / most obvious thing that's different about this element to me.
Test Plan: Viewed normal comments locally, faked a comment on an empty line in the middle of a lot of other empty lines.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15864
Summary:
Ref T10694.
- Shift margins/padding around so inlines with multiple paragraphs get reasonable spacing.
- Add `text-decoration: none` to the "View Inline" link to kill the underline.
Test Plan: {F1265342}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15863
Summary: Ref T10694. Move the inline style more toward a mix of standard`<pre>` style and the web UI style for inlines.
Test Plan: See screenshots in comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15857
Summary: Fixes T10928. This behavior hasn't changed at all and this is what, e.g., `git push --mirror` means, and the other behavior isn't possible, but be more explicit about the inherent destructive potential of mirroring.
Test Plan: Read documentation.
Reviewers: chad, sascha-egerer
Reviewed By: sascha-egerer
Subscribers: sascha-egerer
Maniphest Tasks: T10928
Differential Revision: https://secure.phabricator.com/D15861
Summary:
Ref T10927. Pretty sure the issue is:
- User makes an HTTPS request.
- Load balancer terminates it, but with an `X-Forwarded-Proto` header.
- `secure001` (or whatever; acting as web host) proxies it to `secure002` (or whatever; acting as a repository host). **This** connection is plain HTTP.
- Since this proxied connection is plain HTTP, we check if the repository can serve over "http", but it can't: only "https". So we fail incorrectly, even though the original user request was HTTPS.
In the long run we should probably forward the `X-Forwarded-Proto` header, but that has some weird implications and it's broadly fine to allow either protocol to serve as long as the other one is active: configuration like `security.require-https` is already stronger than these settings.
Test Plan: This is likely only observable in production, but normal cloning still works locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10927
Differential Revision: https://secure.phabricator.com/D15856
Summary:
Ref T10694. This mostly prevents us from having a degenerate case if someone leaves a 200-line inline.
- For one-line inlines, show 1 line of context above and below (3 lines total).
- For 3+ line inlines, show just the inline.
- For 7+ line inlines, show only the first part.
Test Plan: Made a bunch of weird long/short/different-sized comments, saw reasonble-appearing context in text and HTML mail output.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15853
Summary:
Ref T10694. Ref T9790. When generating inline diff context, highlight it and then mangle the highlighted output into `style="..."` so it works in HTML.
Also try to tighten up some spacing/formatting stuff.
Test Plan:
Got some output in this vein:
{F1259937}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790, T10694
Differential Revision: https://secure.phabricator.com/D15852
Summary:
Ref T10694. This is still missing some pieces, but seems to get most of the data into the mail in a plausible format:
- When an inline remarks on code, show the patch inline in the mail body.
- When an inline replies to another inline, show that other inline in the mail body.
- Apply remarkup rendering to inline content.
- Apply basic styling to mail body blocks.
Not covered yet:
- Syntax highlighting.
- Diff highlighting.
- Maybe clearer style/layout hints to connect comments to what they reply to? Current approach might get messy with inlines that have blockquotes and code blocks inside them, for example.
- I probably want to cap the amount of diff context we ever show to ~7 lines, even if you drag over 200 lines of code.
- CSS is a generally a bit rough still.
- The `unified-comment-context` option is effectively always on now, and should be removed.
- Text section is getting indented right now but probably shouldn't be.
- Spacing, etc., might be a bit off.
Test Plan:
Rigged Home to render these things, got a plausible-looking render (top is text, bottom is HTML):
{F1259052}
Sent myself some inline comment mail, got a plausible result.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15850
Summary: Ref T9790. This passes the map down so we can generate highlighted mail.
Test Plan:
Generated this relatively respectable-looking HTML mail:
{F1258558}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15848
Summary:
Ref T9790. This prepares the syntax color rules to be reused in mail.
This goes about halfway toward T5701 by sort-of supporting different styles but not really.
Test Plan:
- Ran `bin/celerity syntax` to regenerate syntax map.
- Viewed some highlighted code, didn't see any differences.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15846
Summary: Ref T10748. We're returning a `PhabricatorRepositoryURI` here but the code expects an actual `PhutilURI`.
Test Plan:
This should clear this up in production:
```
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] [2016-05-04 23:25:16] EXCEPTION: (PhutilProxyException) Error while executing Task ID 1677075. {>} (RuntimeException) Object of class PhabricatorRepositoryURI could not be converted to string at [<phutil>/src/error/PhutilErrorHandler.php:205]
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] arcanist(head=master, ref.master=c58f1b9a2507), libcore(), phabricator(head=master, ref.master=29d1115037b8), phutil(head=master, ref.master=0709cd5cfc26), services(head=master, ref.master=04ae8c8f8e3b)
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository.php:1200]
Daemon 180278 STDE [Wed, 04 May 2016 23:25:16 +0000] #1 <#2> PhabricatorRepository::getPublicCloneURI() called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryCommit.php:395]
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15844
Summary:
Ref T10748. This needs more extensive testing and is sure to have some rough edges, but seems to basically work so far.
Throwing this up so I can work through it more deliberately and make notes.
Test Plan:
- Ran migration.
- Used `bin/repository list` to list existing repositories.
- Used `bin/repository update <repository>` to update various repositories.
- Updated a migrated, hosted Git repository.
- Updated a migrated, observed Git repository.
- Converted an observed repository into a hosted repository by toggling the I/O mode of the URI.
- Conveted a hosted repository into an observed repository by toggling it back.
- Created and activated a new empty hosted Git repository.
- Created and activated an observed Git repository.
- Updated a mirrored repository.
- Cloned and pushed over HTTP.
- Tried to HTTP push a read-only repository.
- Cloned and pushed over SSH.
- Tried to SSH push a read-only repository.
- Updated several Mercurial repositories.
- Updated several Subversion repositories.
- Created and edited repositories via the API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15842
Summary:
Ref T10748. This migrates and swaps mirroring to `PhabricatorRepositoryURI`, obsoleting `PhabricatorRepositoryMirror`.
This prevents you from editing, adding or disabling mirrors unless you know a secret URI (until the UI cuts over fully), but existing mirroring is not affected.
Test Plan:
- Added a mirroring URI to an old repository.
- Verified it worked with `bin/repository mirror`.
- Migrated forward.
- Verified it still worked with `bin/repository mirror`.
- Wow, mirroring: https://github.com/epriestley/locktopia-mirror
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15841
Summary:
Ref T10748. In D14250#158181, I accepted this conditional on removing it once Conduit could handle it.
Conduit can now handle it, or at least will be able to as soon as T10748 cuts over.
Test Plan: Grepped for `repository edit`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15839
Summary:
Ref T10748. This has had many problems for a long time, can't create hosted repositories, can't create cluster repositories, etc. It is obsoleted by `diffusion.repository.edit`. Remove it.
(Right now `diffusion.repository.edit` isn't a strict replacement, but it will be as soon as the URI stuff cuts over.)
Test Plan: Grepped for `repository.create`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15838
Summary:
Ref T4039. Long ago these were more freely editable and there were some security concerns around creating a repository, then setting its local path to point somewhere it shouldn't.
Local paths are no longer editable so there's no real reason we need to provide a uniqueness guarantee anymore, but you could still make a mistake with `bin/repository move-paths` by accident, and it's a little cleaner to pull them out into their own column with a key.
(We still don't -- and, largely can't -- guarantee that two paths aren't //equivalent// since one might be symlinked to the other, or symlinked only on some hosts, or whatever, but the primary value here is as a sanity check that you aren't goofing things up and pointing a bunch of repositories at the same working copy by mistake.)
Test Plan:
- Ran migrations.
- Grepped for `local-path`.
- Listed and moved paths with `bin/repository`.
- Created a new repository, verified its local path populated correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D15837
Summary: Seems to work ok, if you give `size=wide` to an image file, we blow it out a bit in DocumentPro mode.
Test Plan:
Test in Phame and Maniphest.
{F1256717}
{F1256718}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15840
Summary:
Ref T10748. These:
- Look nice.
- Hint at panel contents / effects.
- Hint which panels have been customized.
- Allow panels with issues or errors to be highlighted with an alert/attention icon.
Test Plan: {F1256156}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15836
Summary: Ref T10748. This copies existing code in the `CreateController` which will eventually be removed.
Test Plan:
- Created a new repository with the EditPro workflow.
- Saw it come up into the cluster properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15835
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: This is misspelled.
Test Plan: Consulted a dictionary.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15827
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:
Fixes T10905. This reverts D15823, which didn't work well for tasks with very long titles (the title would break as a block element).
This is slightly more magic but works with long titles.
Test Plan: Did everything from D15823, but also with long titles. Triple-click, wrapping, and mobile/device worked in Safari, Firefox and Chrome.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10905
Differential Revision: https://secure.phabricator.com/D15824
Summary:
Fixes T10905. In Firefox, triple clicking the new headers doesn't select the entire line, so you can't easily copy/paste an entire task title or revision name. It works fine in Safari/Chrome.
This seems to fix that without breaking anything.
Test Plan:
- Viewed headers in Safari, Firefox, Chrome.
- Triple-clicked headers in Safari, Firefox, Chrome.
- Viewed tablet/device layouts in Safari, Firefox, Chrome.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10905
Differential Revision: https://secure.phabricator.com/D15823
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:
Ref T10866. Fixes T10386. This attempts to make it a little more plausible to follow these directions:
- Use simpler language in general.
- Remove language suggesting that HTTP requires no additional configuration.
- Suggest using a load balancer or an ugly port number instead of swapping SSH to a different port.
- Be more granular about `sudo` setup.
- Organize better?
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10386, T10866
Differential Revision: https://secure.phabricator.com/D15796
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
