Summary:
Fixes T9323. Two minor fixes:
- On the first commit, don't render a downward line.
- Clean up a 1px spacing issue that had cropped up a while ago when we added icons or something, I think.
Test Plan:
Before:
{F1057248}
After:
{F1057249}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9323
Differential Revision: https://secure.phabricator.com/D14974
Summary:
Fixes T9319. Proxied requests (e.g., in the cluster) for binary files (like images) currently fail because we can not return binary data over Conduit in JSON.
Although Conduit will eventually support binary-safe encodings, a cleaner approach to this is just to return a `filePHID` instead of the raw content. This is generally faster and more flexible, and gives us more opportunities to add caching later.
After making the call, the client pulls the file data separately.
We also no longer need to return a complex data structure because we don't do blame over this call any longer.
Test Plan:
- Viewed images in Diffusion.
- Viewed READMEs in Diffusion.
- Used `bin/differential attach-commit rX Dy` to hit attach pathway.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14970
Summary: Ref T9319. Ref T2783. This won't currently work in a future environment where daemons and repositories are not on the same host. Send it over Conduit instead.
Test Plan: Used `bin/differential attach-commit rX Dy` to force attachment, saw valid content pull over Conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2783, T9319
Differential Revision: https://secure.phabricator.com/D14969
Summary:
Ref T9319. See D14967. As before, this is making a deeply-buried, complex operation easier to test by providing a CLI command.
This adds `bin/differential attach-commit rXnnnn Dnnnn` to pretend that `rXnnnn` was just committed and matched `Dnnnn`.
Test Plan:
- Ran `bin/differential attach-commit X Y` for several different values, saw updates in the UI.
- Faked the message parser to make sure stuff still worked there.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14968
Summary: Fixes T9701. I don't want to try to autofix this because destroying the directory could destroy important files, but we can improve the error message.
Test Plan: Faked a failure, ran `repository update X`, got a more tailored error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9701
Differential Revision: https://secure.phabricator.com/D14971
Summary:
The old treatment was fairly technical. Give this UI a more human-friendly flow:
- Use language "remove" instead of "disable". We keep the record that the card existed around for auditing/historical purposes, but it is no longer a valid payment method going forward and can not be undone. I think this aligns with user expectation and actual behavior better than "disable".
- Only show active methods on the profile screen.
Test Plan: {F1057153}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14973
Summary:
Ref T9319. When we discover a commit, we sometimes update the corresponding revision with a "this is the actual committed change" diff and send out a link to the changes between review and commit.
This is currently very difficult to test, because it only happens the first time and you have to either go set up a bunch of objects or add a bunch of special casing to the parser to hit the workflow.
I'm making some changes to how it pulls file content. To make those changes easier to test, first start extracting this stuff so the code can be run with `bin/differential extract ...` instead of needing to do a bunch of more complicated setup steps.
Test Plan:
- Ran `bin/differential extract ...` to extract diffs from commits.
- Forced my way through the daemon workflow by faking out a bunch of flags, got a clean extract + attach + update. After this patch, this should rarely be necessary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14967
This is a likely fix for HTTP clones against proxied repositories in the
cluster, although I'm not 100% sure I'm replicating it correctly.
The issue appears to be that we're proxying all the headers, including the
"Transfer-Encoding" header, although the request will already have stripped
any encoding. This might cause us to emit a "chunked" header without a
chunked body.
Auditors: chad
Summary: I was looking at some random un-revisioney repository for most of my testing and missed these.
Test Plan: Viewed blame of a file with some revisions.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14965
Summary: Fixes T2450. If we spend more than 15 seconds in blame, just cut it off.
Test Plan:
- Changed timeout to 0.01 seconds.
- Did blame on a non-highlighted file, got no blame, saw warning.
- Did blame on a highlighted file, got no blame.
- Note: you don't get a warning here because of Ajax stuff. It'd be kind of tricky to add and doesn't seem like a big deal so I'm planning to leave it as-is for now.
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4, chasemp
Maniphest Tasks: T2450
Differential Revision: https://secure.phabricator.com/D14964
Summary:
Ref T2450. This reorganizes code to improve performance.
Mostly, there are a lot of things which are unique per commit (author name, links, short name, etc), but we were rendering them for every line.
This often meant we'd render the same author's name thousands of times. This is slower than rendering it only once.
In 99% of interfaces this doesn't matter, but blame is weird and it's significant on big files.
Test Plan:
Locally, `__phutil_library_map__.php` now has costs of roughly:
- 550ms for main content (from 650ms before the patch).
- 1,500ms for blame content (frrom 1,800ms before the patch).
So this isn't huge, is a decent ~20%-ish performance gain for shuffling some stuff around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2450
Differential Revision: https://secure.phabricator.com/D14963
Summary:
Ref T2450. File blame tends to have the same commit a lot of times, and we don't do lookups like this efficiently right now.
In particular, for a file like `__phutil_library_map__.php`, we would issue a query with ~9,000 clauses like this:
```
(repositoryID = 1 AND commitIdentifier LIKE "XYZ%")
```
...but only a few hundred of those identifiers were unique. Instead, issue only one clause per unique identifier.
MySQL also seems to do a little better on "commitIdentifier = X" if we have the full hash, so special case that slightly.
Test Plan:
- Issuing a query for only unique identifiers dropped the cost from 400ms to 100ms locally.
- Swapping to `=` if we have the full hash dropped the cost from 100ms to 75ms locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2450
Differential Revision: https://secure.phabricator.com/D14962
Summary:
Fixes T4366. Two years ago, Facebook put 16,000 files in a directory. Today, the page has nearly loaded.
Paginate large directories.
Test Plan:
- Viewed home and browse views in Git, Mercurial and Subversion.
I put an artificially small page size (5) on home:
{F1055653}
I pushed 16,000 files to a directory and paged through them. Here's the last page, which rendered in about 200ms:
{F1055655}
Our behavior is a bit better than GitHub here, which shows only the first 1,000 files, disables pagination, and can't retrieve history for the files:
{F1055656}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4366
Differential Revision: https://secure.phabricator.com/D14956
Summary: Make sure to subclass the right controller on badges.
Test Plan: arc liberate, make a custom badges edit form.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14961
Summary:
When looking at a large file in Diffusion:
- disable highlighting if it's huge and show a note about why;
- pick up a few other optimizations.
Test Plan: Locally, this improves the main render of `__phutil_library_map__.php` from 3,200ms to 600ms for me, at the cost of syntax highlighting (we can eventually add view options and let users re-enable it).
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14959
Summary:
Ref T2450. Ref T9319. This is still a bit messy, but not quite so bad as it was: instead of using a single call to get both blame information and file content, use `diffusion.blame` for blame information.
This will make optimizations to both blame and file content easier.
Test Plan: Viewed a bunch of blame (color on/off, blame on/off).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2450, T9319
Differential Revision: https://secure.phabricator.com/D14958
Summary:
Fixes T2451. Several motivations here, from strongest to weakest:
- Currently, getting blame and file content are closely entwined. This makes fixing T9319 more difficult, and I want to fix it. I want to separate blame from content so there's more flexibility in how we approach this issue.
- This makes pursuing T2450 easier, if it turns out to be a meaningful win.
- If we can get a win on blame performance, we can do `arc blame` eventually if we want.
Test Plan:
- Blamed in SVN, Git and Mercurial.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2451
Differential Revision: https://secure.phabricator.com/D14957
Summary:
Fixes T10089. This did work at one point, but was broken by D12868, which got too aggressive about mailing members.
We don't want to send mail to all members by default, only those who are subscribed. The parent implementation of `getMailCC()` handles this for us.
Test Plan:
Joined a project as users A and B. Unsubscribed with B. Made an edit.
Before patch: both A and B got mail. After patch: only A got mail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10089
Differential Revision: https://secure.phabricator.com/D14955
Summary: Adds a basic HeraldAdapter to Phame Blogs and Posts.
Test Plan: Make a Herald rule to CC me on new posts or blogs automatically.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14897
Summary: Fixes T9191. This is pretty fluff but doesn't hurt anything, I guess.
Test Plan: Viewed repository list, saw an importing repository get a little icon.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9191
Differential Revision: https://secure.phabricator.com/D14950
Summary: Ref T4245. This is the last of it, and covers the clone/push stuff.
Test Plan:
- Cloned git.
- Pushed git.
- Cloned mercurial.
- Pushed mercurial.
- Visited a `blah.git` URL in my browser just because; got redirected to a human-facing UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14949
Summary: Ref T4245. On their best day these don't work all that well, but I'm pretty sure I didn't make anything worse.
Test Plan:
- Viewed global lint.
- Viewed lint for a repository.
- Viewed lint details for a particular message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14948
Summary: Ref T4245. This gets everything else except serving HTTP requests (complicated) and lint (quite weird).
Test Plan:
- Viewed a diff.
- Viewed externals.
- Viewed history table to see last modified.
- Did path completion and validation in Owners.
- Did tree path search in Diffusion.
- Viewed a repository.
- Created a new repository.
- Looked up symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14947
Summary: Fixes T7852. Although `1` could also indicate other kinds of problems, assume it means "no results".
Test Plan: Searched for nonsense strings in Git and Mercurial. Searched for valid strings in Git and Mercurial.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7852
Differential Revision: https://secure.phabricator.com/D14943
Summary:
Ref T4245. Browsing is huge and currently split across 5 files using controller delegation.
Although having a huge file isn't great, I think the way it is split up is currently worse, and it gets weird with more flexible repository identifiers.
So this is mostly merging five controllers into one, then a bit of modernization.
I think this can probably be split up better by pulling some of it out into views, instead of using delegation.
Test Plan: Browsed files, directories, and search results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14942
Summary: Ref T4245. Prepares these controllers to accept alternate identifers, plus minor spacing and layout fixes.
Test Plan: Viewed tags, viewed branches.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14941
Summary:
Ref T4245. This adds support for both ID-based and callsign-based routes, although the ID-based routes don't occur anywhere.
Also moves toward simplifying the DiffusionRequest stuff.
Test Plan: Visited normal callsign-based commit pages; visited new ID-based commit pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14940
Summary: Ref T4245. This further reduces the reliance on callsigns in Diffusion.
Test Plan:
- Pretty reasonable test coverage already exists.
- Browsed repository list, browse view, history view, content view, change view, commit view, tag view, branch view of repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14937
Summary:
Ref T10077. Ref T8918. The way the main menu is built is not very modular and fairly hacky.
It assumes menus are provided by applications, but this isn't exactly true. Notably, the "Quick Create" menu is not per-application.
The current method of building this menu is very inefficient (see T10077). Particularly, we have to build it //twice// because we need to build it once to render the item and then again to render the dropdown options.
Start cleaning this up. This diff doesn't actually have any behavioral changes, since I can't swap the menu over until we get rid of all the other items and I haven't extended this to Notifications/Conpherence yet so it doesn't actually fix T8918.
Test Plan: Viewed menus while logged in, logged out, in different applications, in desktop/mobile. Nothing appeared different.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8918, T10077
Differential Revision: https://secure.phabricator.com/D14922
Summary: Ref T4245. These mostly relate to building URIs.
Test Plan: Tried to hunt down as many of these in the UI as I could. Some are a bit tricky but they should be low-risk.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14933
Summary:
Ref T4245. Like everything else, accept more identifiers.
This needs a change in `arc`, which I've made a note about elsewhere.
Test Plan: Used "Update Now" from web UI, saw update get scheduled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14932
Summary: Ref T4245. Pass the whole repository in so it can do something else in a future change.
Test Plan: Loaded changesets in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14931
Summary:
Ref T4245. This was obsoleted long ago and has no callers in Phabricator or Arcanist.
Also some minor cleanup.
Test Plan: `grep` for callers everywhere.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14930
Summary: Ref T4245. More of the same, just narrowing down the easy cases.
Test Plan:
- Called `diffusion.querycommit`.
- Browsed branches.
- Browsed repository.
- Browsed directory.
- Searched for stuff.
- Viewed a commit.
- Viewed a file diff.
- Edited a commit.
- Viewed history.
- Viewed tags.
- Viewed push log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14929
Summary:
Ref T4245. Broaden support to include "ABCD", "rABCD", "1234", "R1234", etc.
This doesn't change the old behavior, just accepts more stuff.
Test Plan:
- Browsed Diffusion.
- Made various calls via API console.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14928
Summary: Ref T4245. These are all straightforward to remove.
Test Plan:
- Edited paths in a package.
- Ran `bin/audit delete --repositories ...` with various identifiers.
- Searched by repository for `R3`, `rAAAA` in Harbormaster.
- Did a Herald dry run on a commit.
- Browsed commits, made comments.
- Viewed a Releeph product list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14927
Summary: Ref T4245. These are all descriptive or UI-facing.
Test Plan:
- Ran `bin/repository pull ...` with various identifiers.
- Ran `bin/repository mirror ...` with various identifiers.
- Ran `bin/repository discover ...` with various identifiers.
- Ran `bin/phd debug pull X Y --not Z` with various identifiers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14926
Summary: This logic wasn't quite right.
Test Plan: Hovered over a recognized commit, got a valid hovercard
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14925
When you `getInt()` an array, PHP decides the array has value `1`. This would
cause us to post to blog #1 incorrectly. I didn't catch this locally because
I happened to be posting to blog #1.
Stop us from interpreting array values as `1`, and fix blog interpretation.
This approach is a little messy (projects has the same issue) but I'll see
if I can clean it up in some future change.
Auditors: chad
Summary:
Ref T4245. Prepare these scripts for a callsign-free world. This also makes them more flexible and easier to use.
The following are now valid ways to identify a repository for these scripts: ID (`3`), PHID (`PHID-REPO-wxyz`), R<ID> (`R3`), r<CALLSIGN> (`rSKYNET`), CALLSIGN (`SKYNET`).
In the future, a human-readable label (`skynet`) may also become valid.
Test Plan:
- Ran `bin/repository reparse --all ...` with `rX`, `X`, `3`, `R3`.
- Ran `bin/repository reparse --change ...` with `rXaaa`, including short versions.
- Ran `bin/repository update ...` with `rX`, `X`, `3`, `R3`.
- Ran `bin/repository refs ...` with various identifiers.
- Ran `bin/repository pull ...` with various identifiers.
- Ran `bin/repository mirror ...` with various identifiers.
- Ran `bin/repository mark-imported ...` with various identifiers.
- Ran `bin/repository list`.
- Ran `bin/repository importing ...` with various identifiers and examined output.
- Ran `bin/repository edit ...` with various identifiers.
- Ran `bin/repository discover ...` with various identifiers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14924
Summary: Ref T4245. Before doing any hard work here, we can dramatically reduce the number of things that make calls to `getCallsign()` to make navigating things easier. Almost all of them only care about a monogram, URI, or display name.
Test Plan:
- Searched for `r uniquename` in jump nav.
- Ran `bin/repository reparse --change rXXXyyyyy --trace`, observed query against bad commit table.
- Ran `bin/search index rXXXyyyy --trace --force`, observed proper title when indexing commit.
- Browed repository list, saw proper `rXXX` and appropriate link targets.
- Mentioned `rXXX` in Remarkup, got a link to the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14923
Summary: Moves Badges over to EditEngine
Test Plan: Create a new Badge, Edit a Badge
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14771
Summary: Creates a new next/previous UI for PhamePosts, and adds a setFoot to PHUIDocumentViewPro for future use in other apps.
Test Plan:
Test first, next, last posts on Phame in mobile, desktop, and tablet breakpoints.
{F1050152}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14919
Summary:
Ref T10010.
Currently, milestone subproject have editable icons/colors, but I don't think this is likely to be used much (the expectation is that milestones will be common and homogenous, and it doesn't make much sense to pick different icons for "Sprint 32" vs "Sprint 33", I think).
Locking the icon and color lets us simplify the form, make milestones more distinct, and potentially reuse the color later for other things (e.g., active/future/past or on time / overdue or whatever else) or just give them a special color to make them more visible.
The best argument for retaining this that I can come up with is that certain milestones may be special (e.g., Sprint 19 is a major release?), but you can just name it "Sprint 19 (v3.0!)" or something, which seems pretty good for now.
Also don't show milestones on task browse/list view.
Test Plan: {F1048532}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14912
Summary: Ref T10004. This control doesn't disable visually or behaviorally, e.g. when locked in an EditEngine configuration.
Test Plan:
- Locked field for Projects.
- Reviewed form in EditEngine.
- Created/edited a project.
- Swapped default.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14911
Summary: Cleans up some language, colors, etc.
Test Plan: Write lots of new posts, hide them, edit them, check history.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14914
Summary: Right now you can't create two blogs without a domain name, since it has a unique key on the column. Removing the key.
Test Plan: Create two blogs with no domain name, works as expected. Create two blogs with `cat.dog` as domain name, get duplicate domain error.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14915
Summary:
Ref T10010. I think this is the desired/expected default behavior (e.g., searching for "Maniphest" should find tasks in any subproject or sprint of that project).
I'll probably add an "exact(...)" function later to mean "only the Maniphest superproject, exactly, not any of its children".
Test Plan:
- Added and executed unit tests.
- Ran various queries from the web UI.
- Got sensible-seeming results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14910
Summary:
Ref T10010.
- Default name to "Milestone X".
- Remove policy controls, which have no effect.
- Don't generate slugs for milestones since this is a big pain where they all generate as `#milestone_1` by default (you can add one if you want). I plan to add some kind of syntax like `#parent/32` to mean "Milestone 32 in Parent" later.
- Don't require projects to have unique names (again, 900 copies of "Milestone X"). I think we can trust users to sort this out for themselves since modern Phabricator has "Can Create Projects" permission, etc.
Test Plan: Created some milestones, had a less awful experience.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14909
Summary:
Ref T10010. This has a lot of UI/UX problems but I think it:
- technically allows subproject creation;
- technically allows milestone creation;
- doesn't let users unwittingly destroy their installs (probably).
Test Plan:
- Created milestones.
- Created subprojects.
- Created and edited normal projects.
- Observed some reasonable interactions (e.g., you can't create milestones for a milestone or edit a superproject's members).
- Observed plenty of silly/confusing interactions that need additional work.
{F1046657}
{F1046658}
{F1046655}
{F1046656}
{F1046654}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14904
Summary: We're checking for drafts even though we already know there are no blogs, just skip the query.
Test Plan: trucate phame_blogs; See proper blank state.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14908
Summary: Fixes T10058. We don't need to continue on this check if no path changes are being applied.
Test Plan: Archived an owners package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10058
Differential Revision: https://secure.phabricator.com/D14906
Summary:
Fixes T10057. Root issue is:
- In the past, you could give tokens to objects of type X (here, Ponder answers).
- Now, you can't.
- If you try to load a token on an object of type X, we do a bad call to attach it and fatal.
Instead, make sure objects implement the proper interface before we attach them, and just pretend the token does not exist otherwise.
Test Plan: Faked the exception in T10057, applied patch, got clean tokens page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10057
Differential Revision: https://secure.phabricator.com/D14905
Summary:
Ref T9897. This one is a little more involved because of how getting a post on a blog works.
I also changed moving posts to be a real transaction (which shows up in history, now).
Test Plan: Created posts from web UI and conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14902
Summary: Ref T9897. Mostly straightforward, but also modernize/fixup the Query a little so that posts never load with no blog.
Test Plan: Queried posts via API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14901
Summary: Ref T9897. Adds basic blog query support.
Test Plan: Ran some queries.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14900
Summary: Ref T9897.
Test Plan: Used API to make a few changes to a blog.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14899
Summary: Ref T9897. We can now provide modern `search` and `edit` endpoints (I'll do this next).
Test Plan: Grepped for removed methods.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14898
Summary: Allows create and edit workflows through EditEngine. Not sure I did the 'blog' stuff correct.
Test Plan: Create a new post, edit a post, move a post.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14802
Summary: Ref T10010. This is pretty straightforward with a couple of very minor new behaviors, like the icon selector edit field.
Test Plan:
- Created projects.
- Edited projects.
- Saw "Create Project" in quick create menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14896
Summary:
Ref T10010. Three motivations:
- Primarily: this makes conversion to EditEngine easier since I don't have to convert this weird control.
- This probably needs to have "Lock", "Unlock" and "Use Parent Project Setting" values after subprojects? But maybe just locking any parent locks all the children? Anyway, doesn't make sense to put it on the main edit form if it's weird like this, I think, since we'll want some kind of explanatory text.
- I probably want to move this to the "Members" tab anyway, and this won't be available on milestone projects at all.
Test Plan: Locked, unlocked, edited projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14895
Summary:
Ref T10010. This will allow us to find superprojects with `withMemberPHIDs(...)` queries.
- Copy all the current real member edges to materialized member edges.
- Redirect all reads to look at materialized members.
- This table is already kept in sync by earlier work with indexing.
Basically, flow is:
- Writes (joining, leaving, adding/removing members) write to the real member edge type.
- After a project's members change, they're copied to the materialized member edge type for that project and all of its superprojects.
- Reads look at materialized members, so "Parent" sees the members of "Child" and "Grandchild" as its own members, but we still have the "real members" edge type to keep track of "natural" or "direct" members.
Test Plan:
- Ran migration.
- Ran unit tests.
- Saw the same projects as projects I was a member of.
- Added some `var_dump()` stuff to verify the Owners changed.
- Used `grep` to look for other readers of this edge type.
- Made some project updates.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14893
Summary:
Ref T10010. When the viewer is logged-out or omnipotent, we can skip this query.
(Currently we issue a silly query like `src = X AND type = Y AND dst = ''`, which will never return results.)
Test Plan:
- Viewed projects as normal user and logged-out user.
- Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14892
Summary: Ref T10010. This is primarily to make "Parent > Child > Grandchild" navigation more manageable for subprojects, at least for now.
Test Plan: Viewed profile, members, feed; saw crumbs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14891
Summary: Ref T10010. Several controlers currently have similar logic for handling tags and slugs, loading projects, and canonicalizing URIs. Clean it up a bit.
Test Plan:
- Visited profile, boards, feed.
- Visited by ID and by tag.
- Visited by non-normal tag (redircted).
- Visited by alternate tag (redirected).
- Visited non-policy project by non-normal tag (redirected into policy error).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14890
Summary: Ref T10010. This still needs support for attachments (to get members) and more constraints (like slugs), but mostly works.
Test Plan: Ran query, saw basically sensible results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14889
Summary:
Ref T10010. We currently require `withSlugs()` to have properly formatted slugs, but this leads to similar code in several places.
Instead: accept any slug, normalize slugs in the query, return a map so callers can figure out what happened if they want.
This tends to do the right thing by default, while keeping enough information around to do more complex things if necessary. A similar approach for querying commits has worked well in Diffusion.
Test Plan: Added and executed unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14888
Summary: Ref T10004. This primarily supports moving Phame to EditEngine.
Test Plan: {F1045166}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14887
Summary:
Ref T9994.
- Allow errors to be dismissed.
- Tailor messaging for closed/abandoned revisions.
- Reduce scare messaging on land dialog, since it's not really that scary anymore.
Test Plan:
- Dismissed errors.
- Hit new warnings.
- Wasn't as scared when landing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9994
Differential Revision: https://secure.phabricator.com/D14886
Summary: This is linking to the wrong article.
Test Plan: O.O
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14885
Summary: I'll revert this within 24 hours, before I promote.
Test Plan: Had completely functional but greatly enhanced Phabricator experience in Firefox, Safari and Chrome.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14884
Summary: Fixes T8482, or something. I can't actually repro that but I think it should be fixed either here or earlier
Test Plan:
```
$ ./bin/lipsum generate paste
GENERATORS Selected generators: Pastes.
WARNING This command generates synthetic test data, including user accounts. It is intended for use in development environments so you can test features more easily. There is no easy way to delete this data or undo the effects of this command. If you run it in a production environment, it will pollute your data with large amounts of meaningless garbage that you can not get rid of.
Are you sure you want to generate piles of garbage? [y/N] y
LIPSUM Generating synthetic test objects forever. Use ^C to stop when satisfied.
Generated "Paste": P223 forgotten_memory_disks_backup.java
Generated "Paste": P224 backup_disk_tables_and_administrate_backup_memory_account.java
Generated "Paste": P225 sync_backup_disk_and_undo_memory.php
Generated "Paste": P226 administrate_memory_shard_helper.php
Generated "Paste": P227 cancel_disk_users
Generated "Paste": P228 backups_pro.txt
Generated "Paste": P229 undo_host.txt
Generated "Paste": P230 accelerate_database_accounts.java
Generated "Paste": P231 entomb_accounts.java
Generated "Paste": P232 legendary_legendary_shards_helper.java
Generated "Paste": P233 compact_backup_and_user_and_purge_memory
Generated "Paste": P234 account_script_script_backup_helper_helper.java
Generated "Paste": P235 purge_disk.php
Generated "Paste": P236 forgotten_elder_account.txt
Generated "Paste": P237 ancient_ancient_disks.txt
Generated "Paste": P238 disk_user.php
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8482
Differential Revision: https://secure.phabricator.com/D14883
Summary: Ref T8980. Previously, if you had like `T123T123T123` and waved your mouse over them, we wouldn't move the card. Now, move the card.
Test Plan: Waved mouse. Saw the card move.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14881
Summary: Ref T8980. This calculation was not quite right and you could sneak one off screen if you tried carefully.
Test Plan: Couldn't sneak one off screen anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14880
Summary:
Ref T8980. Move away from events to EngineExtensions.
This also simplifies hovercards a bit:
- Removes tasks from revision cards.
- Removes blockers/blocked from task cards.
- Removes "Send Message" from user cards.
These mostly felt cluttery to me. Open to arguments to retain them. I think we can make better use of the space, though (e.g., flags, projects + board columns).
Test Plan:
- Viewed people, task, revision, commit and project hovercards.
{F1043256}
{F1043257}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14878
Summary:
Ref T8980. This isn't 100% coverage but should be pretty much all of the common ones.
These feel a touch iffy to me at first glance so I didn't go crazy trying to hunt all of them down. I have some other plans for them so maybe they'll feel better by the end of it.
Test Plan: Hovered over author, reviewers, blocked tasks, projects, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14877
Summary: Ref T10032. This is sufficent to hit NUX without doing anything bad.
Test Plan:
- Visited NUX.
- Browsed normally.
{F1043191}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14876
Summary:
After certain types of errors, we may deadlock when trying to destroy test databases.
Specifically, we still have connections open to, say, `phabricator_unittest_abasonaknlbaklnasb_herald` (or whatever) and MySQL sometimes (not sure exactly when?) waits for them before destorying the database.
Test Plan:
- Added `$m = null; $m->method()` to a fixture test to force a fatal.
- Saw consistent deadlock, with `storage destroy` never exiting.
- Added `--trace` to the `storage destroy` command and made it use `phutil_passthru()` so I could see what was happening.
- Saw it hang on some arbitrary database.
- Conneced to MySQL, used `show full processlist;` to see what was wrong.
- Saw the `DROP DATABASE ...` command waiting for locks to release on the database, and other connections still open.
- Applied patch.
- Saw consistent success.
- Used `storage destroy --unittest-fixtures` to clean up extra databases.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14875
Summary:
Fixes T9156.
- Fix hashtag generation.
- Fix various badnesses.
- Improve project name generator.
Test Plan:
```
$ ./bin/lipsum generate projects
GENERATORS Selected generators: Projects.
WARNING This command generates synthetic test data, including user accounts. It is intended for use in development environments so you can test features more easily. There is no easy way to delete this data or undo the effects of this command. If you run it in a production environment, it will pollute your data with large amounts of meaningless garbage that you can not get rid of.
Are you sure you want to generate piles of garbage? [y/N] y
LIPSUM Generating synthetic test objects forever. Use ^C to stop when satisfied.
Generated "Project": Self-Flying Data Center Swag Performance
Generated "Project": Optimize Cars
Generated "Project": Triaging Culture Optimization
Generated "Project": Automating Experience
Generated "Project": Accelerating NUX Performance
Generated "Project": Optimizing Culture Optimization
Generated "Project": Optimize Hardware
```
{F1042949}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9156
Differential Revision: https://secure.phabricator.com/D14874
Summary: Ref T9156. This makes the UX a little more modern/standard/safe.
Test Plan:
```
epriestley@orbital ~/dev/phabricator $ ./bin/lipsum generate
Choose which type or types of test data you want to generate, or select "all".
- Differential Revisions
- Files
- Maniphest Tasks
- Pastes
- Pholio Mocks
- Projects
- User Accounts
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9156
Differential Revision: https://secure.phabricator.com/D14873
Summary:
Fixes T9019. Pretty much ripped from D14467. I added the "policy hint" stuff so that you can create a project with this policy immediately.
I really dislike how the "hint" code works, but we //almost// never need to use it and the badness feels fairly well-contained.
Also pick up a quick feedback fix from D14863.
Test Plan:
- Added test coverage, got it to pass.
- Created a project with "Visible To: Project Members".
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9019
Differential Revision: https://secure.phabricator.com/D14869
Summary: Ref T8509. We currently give you a fairly obtuse error when trying to name a project something like "!!". The error is correct, but not as helpful as it could be. Give users a more specific, more helpful error.
Test Plan: {F1042883}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14872
Summary:
Fixes T8509. Changes these behaviors:
- If you create a project named "QQQ" and add "qqq" as a hashtag at the same time, it fails in an unhelpful way. (Now: succeeds.)
- If you add "qqq" as a hashtag to a project with primary hashtag "qqq", it fails in a correct but probably unnecessary way (Now: just works).
We could make one or both of these behaviors show the user an error instead, but I think it's likely that this behavior is just what they always want.
Test Plan:
- Added failing tests and made them pass.
- Executed both scenarios described above from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14871
Summary:
Ref T8509. This fixes three issues:
- Adding a slug like `UPPERCASE` would not give you a normalized slug. (Now: normalizes as `uppercase`.)
- Adding a slug like `UPPERCASE` would allow you to give two different projects the different tags `UPPERCASE` and `uppercase` (and `UpPeRcAsE`, etc). (Now: second tag is rejected as a duplicate.)
- Adding multiple identical or similar slugs would produce a duplicate key exception. (Now: ignores the duplicates.)
Test Plan:
- Added test coverage.
- Made tests pass.
- Hit these cases in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14870
Summary: See D14467. Just teasing this apart so I can be a little more confident in my commandeering. These are the unambiguous cleanup changes from D14467.
Test Plan: inspection / clicked stuff / no impact
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Differential Revision: https://secure.phabricator.com/D14868
Summary: I feel like the daemon configuration should be locked from editing from the web UI, given that much of it won't work unless the daemons are restarted anyway.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14867
Summary:
Ref T10010. Subprojects have the following general membership rule: if you are a member of a subproject ("Engineering > Backend"), you are also a member of the parent project.
It would be unreasonably difficult to implement this rule directly in SQL when querying `withMemberPHIDs()`, because we'd have to do an arbitrarily large number of arbitrarily deep joins, or fetch and then requery a lot of data.
Instead, introduce "materailized members", which are just a copy of all the effective members of a project. When a subproject has a membership change, we go recompute the effective membership of all the parent projects. Then we can just JOIN to satisfy `withMemberPHIDs()`.
Having this process avialable will also be useful in the future, when a project's membership might be defined by some external source.
Also make milestones mostly work like we'd expect them to with respect to membership and visibility.
Test Plan:
- Added and executed unit tests.
- Changed project members, verified materialized members populated correctly in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14863
Summary:
Ref T10010. This adds infrastructure for querying projects by type, depth, parent and ancestor.
I needed to revise the "extended policy check" cycle detection rules. When, e.g., querying a grandchild, they incorrectly detected a cycle because both the child and grandchild needed to check the policy of the grandparent.
Instead, simplify it to just do a basic runaway calldepth check. There are many other safety mechanisms to make it so this can't ever occur.
(Cycle detection does have existing test coverage, and those tests still pass, it just takes a little longer to detect the cycle internally.)
There is still no way to create subprojects in the UI.
Test Plan: Added and executed unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14862
Summary:
Ref T10010. This implements technical groundwork for subprojects. Specifically, it implements policy rules like Phriction:
- to see a project, you must be able to see all of its parents (and the project itself).
- you can edit a project if you can edit any of its parents (or the project itself).
To facilitiate this, we load all project ancestors when querying projects so we can do the view/edit checks.
This does NOT yet implement:
- proper membership rules for these projects (up next);
- any kind of UI to let users create subprojects.
Test Plan:
- Added unit tests.
- Executed unit tests.
- Browsed Projects (no change in behavior is expected).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14861
Summary:
Ref T10010. Currently, we do an unusual JOIN to make testing for viewer membership in projects a little cheaper.
This won't work as-is once we have subprojects, so standardize, simplify, and cover it with more tests for now. (I may be able to get a similar optimization later, but want a correct implementation first.)
Test Plan:
This change should create no behavioral differences.
- Added tests.
- Ran tests.
- Viewed projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14859
Summary: Ref T5240. We don't currently use any of these options and I don't think we have any plans to use them. Strip them out for now to make fixing drag-and-drop stuff easier.
Test Plan: Grepped for removed stuff, no hits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5240
Differential Revision: https://secure.phabricator.com/D14864
Summary:
Ref T10004. This could sometimes pass `false`, which counts as disabled.
Instead, pass `null` explicitly.
Test Plan: Edited default space on an EditEngine form.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14858
Summary: Currently we do not show node IDs in this view, but do show token IDs in the stream view. Given that this view facilitates testing various XHPAST functionality, it would be useful to add this information.
Test Plan: Saw node IDs in XHPAST.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14697
Summary:
Fixes T1895. Now that we have on-demand prviews, we can use them on mobile. On mobile:
- don't show live previews;
- only save drafts every 10 seconds.
Also, show fewer remarkup buttons on mobile to try to make sure the more important ones (preview, e.g.) fit.
Test Plan:
- Made window narrower and wider to trigger preview/no-preview behavior.
- Used DarkConsole to verify request rate.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1895
Differential Revision: https://secure.phabricator.com/D14856
Summary:
Ref T3967. This gives us a reasonable baseline for doing remarkup previews inline in all contexts, and works in weird/constrained context including:
- inline comments;
- conpherence; and
- custom fields.
It would be nicer to go beyond this in contexts like Phame posts, but this is a start, at least.
Test Plan:
{F1040877}
{F1040878}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3967
Differential Revision: https://secure.phabricator.com/D14855
Summary: Adds a basic nux for `/` and not found documents. Ref T10023
Test Plan: Visit a clean install, see Welcome page. Visit a non built page, see not found UX
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10023
Differential Revision: https://secure.phabricator.com/D14854
Summary: These transactions (when a user subscribes or unsubscribes only themselves) are universally uninteresting.
Test Plan:
- Subscribed/unsubscribed, saw transactions but no feed/mail.
- Commented, got implicitly subscribed, saw only comment in feed/mail, saw both transasctions on task.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14853
Summary:
- Phurl is missing a ReplyHandler / MailReceiver (all of this code should get cleaned up eventually, but I don't plan to get to it for a while).
- Badges has a bad call.
This should clean up some bad daemon tasks.
Test Plan: Saw fewer daemon errors after these changes.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14852
Summary:
Ref T9979. This uses ngrams (specifically, trigrams) to build a reasonably efficient index for substring matching. Specifically, for a package like "Example", with ID 123, we store rows like this:
```
< ex, 123>
<exa, 123>
<xam, 123>
<amp, 123>
<mpl, 123>
<ple, 123>
<le , 123>
```
When the user searches for `exam`, we join this table for packages with tokens `exa` and `xam`. MySQL can do this a lot more efficiently than it can process a `LIKE "%exam%"` query against a huge table.
When the user searches for a one-letter or two-letter string, we only search the beginnings of words. This is probably what they want, the only thing we can do quickly, and a reasonable/expected behavior for typeaheads.
Test Plan:
- Ran storage upgrades and search indexer.
- Searched for stuff with "name contains".
- Used typehaead and got sensible results.
- Searched for `aabbccddeeffgghhiijjkkllmmnnooppqqrrssttuuvvwwxxyyzz` and saw only 16 joins.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14846
Summary: Adds a no visible blogs and no posts nux state using new UI. Ref T10032
Test Plan: Archived all my blogs, got no posts fallback. Test a New Blog, got create a post, logged out, saw no create button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14848
Summary:
Fixes T10037. When we're building commit `aabbccdd`, we currently do this to check it out:
git reset --hard aabbccdd
However, this has an undesirable side effect of moving the current branch pointer to point at `aabbccdd`. The current branch pointer may be some totally different branch which `aabbccdd` is not part of, so this is confusing and misleading.
Instead, use `git reset --hard HEAD` to get the primary effect we want (destroying staged changes) and then `git checkout aabbccdd` to checkout the commit in a detached HEAD state.
Test Plan:
- Ran a build (a commit-focused operation) successfully.
- Verified working copy was pointed at a detached HEAD afterward:
```
builder@sbuild001:/var/drydock/workingcopy-167/repo/git-test-ii$ git status
HEAD detached at ffc7635
nothing to commit, working directory clean
```
- Ran a land (a branch-foused operation) successfully.
- Verified working copy was pointed at a branch afterward:
```
builder@sbuild001:/core/data/drydock/workingcopy-168/repo/git-test$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working directory clean
```
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10037
Differential Revision: https://secure.phabricator.com/D14850
Summary:
Ref T6884. Ref T10004. For various reasons we previously didn't publish these transactions, but now do. This is probably a better behavior overall, but we didn't have reasonable strings for them.
Parent tasks now show "alice created blocking task Txxx.".
Feed now shows nothing, since "alice created task Txxx." is right next to any story we would show and showing them both seems silly.
Test Plan:
- Created subtasks.
- Viewed parent tasks.
- Viewed feed.
- Saw pretty reasonable strings/stories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6884, T10004
Differential Revision: https://secure.phabricator.com/D14849
Summary:
Fixes T9890. This allows IndexExtensions to emit an object version.
Before we build indexes, we check if the indexed version is the same as the current version. If it is, we just don't call that extension.
T9890 has a case where this is useful: a script went crazy and posted thousands of comments to a single task.
Without versioning, that results in the same comments being indexed over and over again. With versioning, most of the queue could just exit without doing any work.
Test Plan:
- Added a `sleep(1)` to the actual indexing, used `bin/search index --background` to queue up a lot of tasks, ran them with `bin/phd debug task`, saw them complete very quickly with only one actual index operation performed.
- Used `bin/search index --trace` and `bin/search index --trace --background` to observe the behavior of queries against the index version store, which looked sensible.
- Made comments/transactions, saw versions update.
- Used `bin/remove destroy`, verified index versions were purged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9890
Differential Revision: https://secure.phabricator.com/D14845
Summary:
Ref T9979. I picked this name long before the advent of modern "Engine" architecture and it ended up being pretty confusing.
Rename "SearchEngine" (currently: mysql or elasticsearch, used to store and query fulltext indexes) to "FulltextStorageEngine" to make it more clear what it does and disambituate it from ApplicationSearch, which also has a bunch of stuff called "SearchEngine", "SearchEngineExtension", etc.
Test Plan: Grepped for `phabricatorsearchengine`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14843
Summary: Ref T9979. This simplifies/standardizes the code a bit, but mostly gives us more consistent class names and structure.
Test Plan:
- Used `bin/search index --type ...` to index documents of every indexable type.
- Searched for documents by unique text, found them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14842
Summary:
Ref T9979. There are currently some hacks around Conpherence indexing: it does not really use the fulltext index, but its own specialized index. However, it's kind of hacked up so it can get reindexed by the normal indexing pipeline.
Lift it up into IndexEngine, instead of FulltextEngine. Specifically, the new stuff is going to look like this:
- IndexEngine: Rebuild all indexes.
- ConpherenceIndexExtension: Rebuild thread indexes.
- ProjectMemberIndexExtension: Rebuild project membership views.
- NgramIndexExtension: Rebuild ngram indexes.
- FulltextIndexExtension / FulltextEngine: Rebuild fulltext indexes, a special type of index.
- FulltextCommentExtension: Rebuild comment fulltext indexes.
- FulltextProjectExtension: Rebuild project fulltext indexes.
- etc.
Most of this is at least sort-of-in-place as of this diff, although some of the part in the middle is still pretty rough.
Test Plan:
- Made a unique comment in a Conpherence thread.
- Used `bin/search index --force` to rebuild the index.
- Searched for the comment.
- Found the thread.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14841
Summary:
Ref T9979. This is currently hard-coded but can be done in a generic way.
This has one minor behavioral changes: answer text is no longer included in the question text index in Ponder. I'm not planning to accommodate that for now since I don't want to dig this hole any deeper than I already have. This behavior should be different anyway (e.g., index the answer, then show the question in the results or something).
Test Plan:
- Put a unique word in a Maniphest comment.
- Searched for the word.
- Found the task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14837
Summary:
Ref T9979. This event had one weird callsite and no known third-party callers. It can be done more cleanly as an extension, now.
This index is used to allow us to "Group By: Project" in Maniphest without joining into the Projects database.
Test Plan:
- Ran a query with "Group By: Project" in Maniphest.
- Renamed project "Apples" to "Zebras".
- Reloaded page.
- UI properly moved "Zebras" tasks to the bottom of the list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14836
Summary: Ref T9979. This is going to become `FulltextEngine`, but pave the way for that by pulling extensions out of it.
Test Plan:
{F1036624}
- Used `bin/search index Txxx`, saw projects, subscribers and custom fields rebuild in the index.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14835
Summary:
Ref T9890. Ref T9979. Several adjacent goals:
- The `SearchEngine` vs `ApplicationSearchEngine` thing is really confusing. There are also a bunch of confusing class names and class relationships within the fulltext indexing. I want to rename these classes to be more standard (`IndexEngine`, `IndexEngineExtension`, etc). Rename `SearchIndexer` to `IndexEngine`. A future change will rename `SearchEngine`.
- Add the index locks described in T9890.
- Structure things a little more normally so future diffs can do the "EngineExtension" thing more cleanly.
Test Plan:
Indexing:
- Renamed a task to have a unique word in the title.
- Ran `bin/search index Txxx`.
- Searched for unique word.
- Found task.
Locking:
- Added a `sleep(10)` after the `lock()` call.
- Ran `bin/search index Txxx` in two windows.
- Saw first one lock, sleep 10 seconds, index.
- Saw second one give up temporarily after failing to grab the lock.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9890, T9979
Differential Revision: https://secure.phabricator.com/D14834
Summary: Ref T9979. Convert all DestructionEngine behaviors to extensions.
Test Plan:
{F1033244}
Destroyed an object, verifying:
- Herald transcripts were destroyed;
- edges were destroyed;
- flags were destroyed;
- tokens were destroyed;
- transactions were destroyed;
- worker tasks were cancelled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14832
Summary:
Ref T9979. The general shape of "engine" code feels pretty good, and I plan to move indexing to be more in line with other modern engines, with the ultimate goal of supporting subprojects (T10010) and several intermediate goals.
Before moving indexing, clean up Destruction, since some of the new indexes will need destruction hooks and destruction currently has a lot of `instanceof` stuff that should be easy to fix by applying more modern approaches.
Test Plan:
- Used `bin/remove destroy` to destory an Almanac device.
- Verified that properties for the device were destroyed.
- Viewed module panel in UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14831
Summary: Ref T10032, adds "Basic" NUX to more applications.
Test Plan: Visit each with ?nux=true and click on the create link. T10032 is tracking which apps need general modernization to pick up these changes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14847
Summary: Adds basic NUX to Dashboards, Herald, Repositories, Maniphest. Note Herald and Dashboard Panels don't fine the nux for some reason, assume they will when modernized?
Test Plan: Read text, click buttons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14844
Summary:
Ref T3462. If someone works directly on `master`, we currently show "Branch: master (branched from master)" in the UI.
Although this is sort of technically accurate, it is confusing.
Instead, just show "Branch: master" in this situation.
Test Plan: Saw "master" instead of "master (branched from master)".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3462
Differential Revision: https://secure.phabricator.com/D14829
Summary:
This is just putting a hook in that pretty much works. Behavior:
- If you visit `/maniphest/?nux=true`, it always shows NUX for testing.
- Otherwise, it shows NUX if there are no objects in the application yet.
Test Plan: {F1031846}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14828
Summary: Currently it's difficult to notice posts may be much longer than the summary (in most cases, actually), Adds a consistent "Read More" link to the full post. Also made sure "violet" was more consistantly used.
Test Plan:
Review summaries in Phame
{F1031799}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14827
Summary: Ref T9967
Test Plan:
Ran migrations.
Verified database populated properly with PHIDs (SELECT * FROM auth_sshkey;).
Ran auth.querypublickeys conduit method to see phids show up
Ran bin/remove destroy <phid>.
Viewed the test key was gone.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9967
Differential Revision: https://secure.phabricator.com/D14823
Summary:
Ref T10010. This does some cleanups on the schema:
- `viewPolicy`, `editPolicy` and `joinPolicy` were nullable, but should never be `null`. Set them to defaults if they're null, then make the column non-nullable.
- Rename `phrictionSlug` to `primarySlug` and stop adding and removing trailing slashes from it.
- Add new columns to support milestones and non-milestone subprojects.
- Drop very old subprojectPHIDs column. This hasn't done anything in the UI for years and years, and isn't particularly realistic to migrate forward.
The new columns aren't reachable from the UI.
Test Plan:
- Applied patches.
- Grepped for `phrictionSlug`.
- Grepped for `subprojectPHIDs`.
- Created tasks.
- Edited tasks.
- Verified existing tasks still had primary slugs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14825
Summary:
Fixes T10012. The permissions here are little weird: you need edit permission on the //configurations//, not the //engines//. I was checking edit permission on the engines only.
I should possibly make this a bit more consistent, the engine edit permission is just very convenient to use to enforce object create permission right now. I'll likely clean this up after T9789.
Test Plan:
- Tried to reorder forms as a less-privileged user, got proper policy errors.
- Reordered forms normally as a regular user.
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10012
Differential Revision: https://secure.phabricator.com/D14824
Summary: Ref T10004. This lost a couple of fields when I rearranged how descriptions work. Restore them.
Test Plan:
- Viewed "Using HTTP Parameters".
- Everything had nice descriptions.
- No more weird phantom/misleading 'comment' transaction in UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14822
Summary: Ref T10004. I missed these previously, so they didn't work quite right. Restore them to glory.
Test Plan: Edited remarkup and text custom fields on an Owners package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14821
Summary:
Ref T10004. This restores "alice created this task." transactions, but in a generic way so we don't have to special case one of the other edits with an old `null` value.
In most cases, creating an object now shows only an "alice created this thing." transaction, unless nonempty defaults (usually, policy or spaces) were adjusted.
Test Plan: Created pastes, tasks, blogs, packages, and forms. Saw a single "alice created this thing." transaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14820
Summary:
Fixes T9994. Currently, when Drydock can't allocate a new resource because some limit has been reached, it waits patiently for a resource to become available.
It is possible that no resource will ever become available. Particularly with "Working Copy" resources, the new lease may want a copy of `rB`, but the resource may already be maxed out on `rA`.
Right now, no process exists to automatically reclaim the unused `rA`.
When we encounter this situation, try to reclaim one of the other resources if it is just sitting there unused.
Specifically:
- Add a "reclaim" command which means "release this resource //if// it is completely unused".
- Add a `bin/drydock reclaim` to send this command to every active resource.
- When we try to acquire a resource and can't, but only because of some kind of limit / utilization problem, try to release an unused resource to free up some room.
Test Plan:
- Set "Working Copy" resource limit to 1.
- Ran "Test Configuration" in `rA`, which worked.
- Ran "Test Configuration" in `rB`, which hung forever.
- Applied patch.
- Ran "Test Configuration" in `rB`, saw it reclaim the `rA` resource, use the slot, then succeed.
- Ran "Test Configuration" in `rA` again, saw it grab the slot back.
- Ran `bin/drydock reclaim` and saw it reclaim a bunch of old orphaned resources.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9994
Differential Revision: https://secure.phabricator.com/D14819
Summary:
Ref T9994. This fixes the first issue discussed on that task, which is that when a merge fails after "arc land", we would not clean up all the leases properly.
Specifically, when a merge fails, we use `queueTask()` to schedule a followup task. This followup destroys the lease and frees the underlying resource.
However, the default behavior of `queueTask()` is to //not queue tasks// if the parent task fails. This is a reasonable, safe behavior that was originally introduced in D8774, where it kept us from sending too much mail if a task did "send some mail" and then failed a little later on and got retried.
Since I think the default behavior is correct, I just special cased the behavior for Drydock to make it queue even on failure. These are the only types of followup tasks we currently want to queue on main task failure.
(It's possible that future Blueprints might want some kind of more specialized behavior, where some tasks queue only on success, but we can cross that bridge when we come to it.)
Test Plan:
- See T9994#149878 for test case setup.
- I ran that test case again with this patch, and saw the followup task queue properly in the `--trace` log, a correspoinding update task show up in `/daemon/`, and the lease get destroyed when I ran it a moment later.
{F1029915}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9994
Differential Revision: https://secure.phabricator.com/D14818
Summary: Ref T10003. Give installs more warning about these changes.
Test Plan: Changed configuration, saw warning. Reset configuration, no warning.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10003
Differential Revision: https://secure.phabricator.com/D14817
Summary: Ref T10004. Happy to take another approach here or just not bother, this just struck me as a little ambiguous/confusing.
Test Plan:
Before, not necessarily clear that the "Create Task" header only applies to the first few items.
{F1029126}
After, more clear:
{F1029127}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14815
Summary: Fixes T9871. Ref T10004. These won't win any awards but it fixes them being incredibly weird and confusing.
Test Plan:
{F1029090}
- Tried to use controls, got reasonable behavior.
- Used normal controls to make sure I didn't break anything.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9871, T10004
Differential Revision: https://secure.phabricator.com/D14814
Summary:
At least for now, the "Space" field is just a subfield of the "Visible To" field, so:
- it doesn't get any separate settings; and
- it always uses the "Visible To" settings.
Test Plan:
- Created a form with a hidden view policy field.
- Created stuff with no "you must pick a space" errors.
- Created stuff with a normal form.
- Prefilled "Space" on a noraml form.
- Verified that trying to prefill "Space" on a form with "Visible To" hidden does nothing.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14812
Summary:
Ref T10004. Fixes T9527. Currently, we render two kinds of bad policy/space transactions during object creation.
First, we render a transaction showing a change from the default policy/space to the selected policy/space:
> alice shifted this object from space S1 Default to space S2 Secret.
This is a //good transaction// (it's showing that the default was changed, which could be important for policy stuff!) but it's confusing because it makes it sound like the object briefly existed in space S1, when it did not.
Instead, render this:
> alice created this object in space S2 Secret.
This retains the value (show that the object was created in an unusual space) without the confusion.
Second, when you create a "New Bug Report", we render a transaction like this:
> alice changed the visibility of this task from "All Users" to "Community".
This is distracting and not useful, becasue it's a locked default of the form. This was essentially fixed by D14810. The new behavior is to show this, //only// if the value was changed from the form value:
> alice created this object with visibility "Administrators".
This should reduce confusion, reduce fluff in the default cases, and do a better job of calling out important changes (basically, unusual spaces/policies).
Test Plan:
- Created an edit form with a default space and policies.
- Used that form to create task with:
- same values as form;
- different values from form.
When I changed the form value, I got transactions. When I left it the same, I didn't.
The transactions rendered in the non-confusing "created with ..." variant.
Editing the values created normal transactions with "changed policy from X to Y".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9527, T10004
Differential Revision: https://secure.phabricator.com/D14811
Summary:
Fixes T7661. Ref T9527.
When you create a task, especially with an EditEngine form, you currently get more noise than is useful. For example:
> alice created this task.
> alice changed the edit policy from "All Users" to "Community (Project)".
> alice added projects: Feature Request, Differential.
> alice added a subscriber: alice.
Transaction (1) is a little useful, since it saves us from a weird empty state and shows the object creation time.
Transaction (2) is totally useless (and even misleading) because that's the default policy for the form.
Transaction (3) isn't //completely// useless but isn't very interesting, and probably not worth the real-estate.
Transaction (4) is totally useless.
(These transactions are uniquely useless when creating objects -- when editing them later, they're fine.)
This adds two new rules to hide transactions:
- Hide transactions from object creation if the old value is empty (e.g., set title, set projects, set subscribers).
- Hide transactions from object creation if the old value is the same as the form default value (e.g., set policy to default, set priorities to default, set status to default).
NOTE: These rules also hide the "created this object" transaction, since it's really one of those transaction types in all cases. I want to keep that around in the long term, but just have it be a separate `TYPE_CREATE` action -- currently, it is this weird, inconsistent action where we pick some required field (like title) and special-case the rendering if the old value is `null`. So fixing that is a bit more involved. For now, I'm just dropping these transactions completely, but intend to restore them later.
Test Plan:
- Created objects.
- Usually saw no extra create transactions.
- Saw extra create transactions when making an important change away from form defaults (e.g., overriding form policy).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7661, T9527
Differential Revision: https://secure.phabricator.com/D14810
Summary:
Ref T10004. Fixes T5158. There was a long-standing issue with defaults not working properly, but EditEngine has made it more obvious because it's a lot easier to set defaults now.
The issue is basically that the defaults are getting set as the field's real value early on, so when we go to generate the transaction "old value" later, we build a transaction that uses the //new// value as both the "new value" and "old value". Then the engine says "you didn't change anything, so I'm going to ignore this" and drops it.
To fix this, return `null` as the "old value" by default, and add a call to overwrite that after we load a legitimate old value.
This fix is a touch iffy, but I have some grand plans to clean up the CustomField stuff more broadly later on.
Test Plan:
- Set config defaults on select/typeahead fields, created and edited tasks.
- Set form defaults on select/typehaead fields, created and edited tasks.
- In all cases, transactions and state accurately reflected edits.
- Set defaults on //hidden// fields, verified forms respected them correctly.
- This does generate some fluffy transactions, but I'll deal with those in T7661.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5158, T10004
Differential Revision: https://secure.phabricator.com/D14809
