Summary: Before the speling pollice lock us in prisun.
Test Plan: Used a dicationairey.
Reviewers: chad, jmeador
Reviewed By: jmeador
Differential Revision: https://secure.phabricator.com/D17570
Summary:
This implements a simplistic `PhabricatorRepositoryFulltextEngine`
Currently only the repository name, description, timestamps and
status are indexed.
Note: I had to change the `search index` workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit
Test Plan:
* ran `./bin/search index --type PhabricatorRepository --force`
* searched for some repositories. Saw reasonable results matching on either title or description.
* Edited a repository in the web ui
* Added unique key words to the repo description.
* I was then able to find that repo by searching for the new keywords.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Tags: #search, #diffusion
Differential Revision: https://secure.phabricator.com/D17300
Summary:
The goal is to make fulltext search back-ends more extensible, configurable and robust.
When this is finished it will be possible to have multiple search storage back-ends and
potentially multiple instances of each.
Individual instances can be configured with roles such as 'read', 'write' which control
which hosts will receive writes to the index and which hosts will respond to queries.
These two roles make it possible to have any combination of:
* read-only
* write-only
* read-write
* disabled
This 'roles' mechanism is extensible to add new roles should that be needed in the future.
In addition to supporting multiple elasticsearch and mysql search instances, this refactors
the connection health monitoring infrastructure from PhabricatorDatabaseHealthRecord and
utilizes the same system for monitoring the health of elasticsearch nodes. This will
allow Wikimedia's phabricator to be redundant across data centers (mysql already is,
elasticsearch should be as well).
The real-world use-case I have in mind here is writing to two indexes (two elasticsearch clusters
in different data centers) but reading from only one. Then toggling the 'read' property when
we want to migrate to the other data center (and when we migrate from elasticsearch 2.x to 5.x)
Hopefully this is useful in the upstream as well.
Remaining TODO:
* test cases
* documentation
Test Plan:
(WARNING) This will most likely require the elasticsearch index to be deleted and re-created due to schema changes.
Tested with elasticsearch versions 2.4 and 5.2 using the following config:
```lang=json
"cluster.search": [
{
"type": "elasticsearch",
"hosts": [
{
"host": "localhost",
"roles": { "read": true, "write": true }
}
],
"port": 9200,
"protocol": "http",
"path": "/phabricator",
"version": 5
},
{
"type": "mysql",
"roles": { "write": true }
}
]
Also deployed the same changes to Wikimedia's production Phabricator instance without any issues whatsoever.
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Tags: #elasticsearch, #clusters, #wikimedia
Differential Revision: https://secure.phabricator.com/D17384
Summary: Ref T5307. This adds an additional action to Use Results for creating a panel from the query.
Test Plan:
Navigate to Maniphest, select dropdown for Use Results. Try any of the following:
- Try to set a panel without a name (fail)
- Muck up query or engine (fail)
- Set a fake Dashboard ID (fail)
Give panel a name and select a dashboard I have edit permissions to, get taken to dashboard.
Reviewers: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17516
Summary: Extends PHUIListItemView to take an icon, link as an "Action Item" that displays on the right side of the menu link. Does not display on Favorites. This allows for adding edit, external, or other links (documentation?) to any menu item. Right now the secondary link is only visible when the item is selected. This feels right, but if we offer it in other ways, users may always want it visible. We could look at making it onhover.
Test Plan:
Add a bunch of random global and personal dashboards to my menu. Add a menu to Favorites, see no link. Test mobile, link works.
{F4136699}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17505
Summary:
Fixes T12400. Adds a "Has MFA" filter to People so you can figure out who you need to harass before turning on "require MFA".
When you run this as a non-admin, you don't currently actually hit the exception: the query just doesn't work. I think this is probably okay, but if we add more of these it might be better to make the "this didn't work" more explicit since it could be confusing in some weird edge cases (like, an administrator sending a non-administrator a link which they expect will show the non-administrator some interesting query results, but they actually just get no constraint). The exception is more of a fail-safe in case we make application changes in the future and don't remember this weird special case.
Test Plan:
- As an administrator and non-administrator, used People and Conduit to query MFA, no-MFA, and don't-care-about-MFA. These queries worked for an admin and didn't work for a non-admin.
- Viewed the list as an administrator, saw MFA users annotated.
- Viewed config help, clicked link as an admin, ended up in the right place.
{F4093033}
{F4093034}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12400
Differential Revision: https://secure.phabricator.com/D17500
Summary:
Ref T12270. This just drops the constraint for now, rather than dealing with all the typecasting stuff and putting us in a position which will almost certainly require backward compatibility breaks in the future.
Also renames "badges.*" to "badge.*" for consistency (all other methods are singular: token.*, project.*, differential.revision.*, etc).
Test Plan:
Saw "qualities" now "Not Supported", while other constraints continue to work:
{F3887194}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17487
Summary:
Ref T12314. Allow tasks to be queried by subtype using a typeahead.
Open to a better default icon. I'll probably let you configure them later.
Just hide this constraint if there's only one subtype.
Test Plan:
- Searched for subtypes.
- Verified that the control hides if there is only one subtype.
{F3492293}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17444
Summary: Ref T12324. Adds back this query for search results in dashboards.
Test Plan: Use panel in Dashboard.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12324
Differential Revision: https://secure.phabricator.com/D17428
Summary: Ref T10390. Simplifies dropdown by rolling out canUseInPanel in useless panels
Test Plan: Add a query panel, see less options.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17341
Summary: I broke this at the last second in D17374. `getStrList()` doesn't read arrays. It probably should (more modern analogs do) but don't rock the boat in the leadup to the release cut.
Test Plan: Hovered over a thing, saw a hovercard and no `getStrList()` error in my logs.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17383
Summary:
Ref T12268. Ref T12157. When you mention or interact with a user who is unlikely to be able to respond (for example, because their account is disabled), we try to show a colored dot to provide a hint about this.
Recently, we no longer send any normal mail to unverified addresses. However, the rules for showing a dot haven't been updated yet, so they only care about this if `auth.require-verification` is set. This can be misleading, because if you say `Hey @alice, what do you think about this?` and she hasn't verified her email, you may not get a response.
Update the rule so users with unverified email addresses get a grey dot in all cases. The hint is basically "you shouldn't expect a response from this user".
Make the meaning of this hint more clear on the hovercard and profile.
Also:
- Allow the non-ajax version of the hovercard page (which is basically only useful for testing hovercards) accept `?names=...` so you can just plug usernames, hashtags, etc., in there.
- Fix a bug where the user's join date was based on their profile creation date instead of account creation date on the hovercard. Users may not have a profile creation date (if they never changed any account details), and it may be different from their account creation date.
Test Plan: {F2998517}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12268, T12157
Differential Revision: https://secure.phabricator.com/D17374
Summary:
Fixes T12281. Some forms (like Settings) can't actually create new objects. Currently, though, you can select them and add them to profile menus; if you do, they fail when building an item.
Kick them out of the typeahead, and decline to render them in menus.
Test Plan:
Added "Create Settings" to a menu, no longer fatals after patch (item vanished from menu, still editable normally to get rid of it).
Tried to add another "Create Settings", no longer available in typehaead.
Added some normal stuff.
Viewed a choose-among-forms dropdown in Maniphest, which still worked normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12281
Differential Revision: https://secure.phabricator.com/D17372
Summary: Ref T12174. Drag-and-drop-to-upload requires some stuff in the document. Put that stuff on all the content pages (currently: dashboards, magic home), not just the builtin home.
Test Plan:
- Dragged-and-dropped onto a Home dashbboard to upload.
- Viewed, and dragged-and-dropped onto "builtin home" to upload.
- Dragged onto "Edit Menu" for home, no upload.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17307
Summary: Ref T5307, Makes these buttons a little more clear visually and verbosely. Adds white icons for blue buttons.
Test Plan: Test saving a search, viewing button changes on various form pages / uiexamples.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17305
Summary:
Ref T12174.
- Home now always uses the topmost item (falling back to "magic home") and no longer supports pinning. If any personal item may be a default item, it will always be picked over any global item.
- Favorites doesn't use defaults anyway, but no longer has misleading UI suggesting it might.
Test Plan:
- Saw no pinning UI on Home/Favorites.
- Added a personal dashboard on Home, it automatically became the new default.
- Pinned stuff normally on Projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17298
Summary:
Ref T12174.
- Go back to the old mobile behavior (full-screen menu by default, click to see content).
- Hide crumbs from all Home content UIs. I left them on the edit/configure UIs since they feel a little less out-of-place there and some have multiple levels.
Test Plan:
Viewed Home on mobile, viewed `/home/` on mobile.
Also, saw no crumbs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17290
Summary: Ref T12174. We were always setting a name via builtins so the tooltip was always set. Fix the calls here.
Test Plan: Add "Badges", see tooltip, give "Badges" a name of "Badges", don't see tooltip.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17284
Summary: Ref T12174. Always sets the correct type when converting to ActionList, adds a type to Divider.
Test Plan:
Add a Label, 2 applications to the personal favorites menu, see nice styles.
{F2554901}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17285
Summary: Ref T12174. Dashboards and "Home" currently use the page title "Configure Menu". Give them more appropriate titles instead.
Test Plan: Viewed dashboards, Home. Saw relevant page titles.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17281
Summary:
Ref T12174. We now require that we can figure out a valid "edit mode" (global vs custom/personal) before we hit EditEngine. Since the EditEngine routes don't have an `itemID`, they would failu to figure out the mode and just 404.
Let the engine use `id` (from EditEngine) if `itemID` (from MenuEngine) isn't present in the route.
Test Plan:
- Edited some menu items on Home / Projects.
- (I think I tested this, then broke it, originally.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17280
Summary: Ref T12174, lets you set labels as well for dividing content.
Test Plan: Add a label, review on homepage.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17278
Summary:
Ref T12174. This fixes more bugs than it creates, I think:
- Dashboards now show the whole menu.
- Project and home items now show selected state correctly.
- The "choose global vs personal" thing is now part of MenuEngine, and the same code builds it for Home and Favorites.
- Home now handles defaults correctly, I think.
Maybe regression/bad/still buggy?:
- Mobile home is now whatever the default thing was, not the menu?
- Title for dashboard content or other items that render their own content is incorrectly always "Configure Menu" (this was preexisting).
Test Plan:
- Created, edited, reordered, disabled, deleted and pinned personal and global items on home, favorites, and projects.
- Also checked User profiles.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17273
Summary: Ref T11957. Needs some more polish, but I think everything here is square.
Test Plan: Add personal/global items to home, test mobile. Test workboards / colors.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: 20after4, rfreebern, Korvin
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17259
Summary:
Fixes T12159. This is similar to D17228, which fixed this for the main configuration operation.
Most other edit operations only test for edit capability on the MenuItem itself, which we already do correctly. However, because reordering affects all items, we test for capability on the object.
Weaken this when reordering custom items.
Test Plan: Reordered custom items in Favorites as a non-administrator.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12159
Differential Revision: https://secure.phabricator.com/D17257
Summary:
Ref T11957. When you click a dashboard item, it now sends you to `/<app>/item/view/123/`, which renders the proper crumbs, navigation, etc., with the dashboard as page content.
This works as you'd expect in Projects:
{F2508568}
It's sliiiightly odd in Favorites since we nuke the nav menu, but seems basically fine?
{F2508571}
Test Plan:
- Created a dashboard panel on a project.
- Clicked it, saw it render.
- Made it the default panel, viewed project default screen, saw dashboard.
- Disabled every panel I could, still saw reasonable behavior (this is silly anyway).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17255
Summary: Fixes T12142. Correct spelling of method.
Test Plan: Edit the name of a Details menu item in projects, or add a divider.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12142
Differential Revision: https://secure.phabricator.com/D17240
Summary:
See T11957#208140.
- Let Applications have a custom name, like other object items (for example, so you can call Maniphest "Tasks" if you prefer).
- Put the optional name field after the required typeahead field for these items.
- (I left "Link" in "Name, URI" order since both are required, but there's maybe an argument for swapping them?)
Test Plan:
- Created each type of item, saw "thing, name" order.
- Created an application with a cusotm name, saw custom name.
- Removed custom name, saw original name.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17236
Summary:
Ref T12128. This adds validation to menu items.
This feels a touch flimsy-ish (kind of copy/paste heavy?) but maybe it can be cleaned up a bit once some similar lightweight modular item types (build steps in Harbormaster, blueprints in Drydock) convert.
Test Plan:
- Tried to create each item with errors (no dashboard, no project, etc). Got appropriate form errors.
- Created valid items of each type.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12128
Differential Revision: https://secure.phabricator.com/D17235
Summary: Removes the often funny, but never really used but will cause us bug reports someday.... cat facts.
Test Plan: Install cat facts, run storage upgrade, see no cat facts in menu.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12126
Differential Revision: https://secure.phabricator.com/D17233
Summary: Mark required fields as required. Though in testing, none of these work.
Test Plan: Try to save a form without an app/project/dashboard and see success (not expected)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17231
Summary:
Ref T11096. Currently, editing ProfileMenuItemConfigurations always requires that you can edit the corresponding object.
This is correct for global items (for example: you can't change the global menu for a project unless you can edit the project) but not for personal items.
For personal items, only require that the user can edit the `customPHID` object. Today, this is always their own profile.
Test Plan: As a non-admin, edited personal menu items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11096
Differential Revision: https://secure.phabricator.com/D17228
Summary:
- Attach objects when showing configuration screen
- Fix "Forms" to make more sense
- Alter EditEngine title to load correct name by loading object
Fixes T12116
Test Plan: Load up Apps/Projects/Forms on a configure menu, see proper names
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12116
Differential Revision: https://secure.phabricator.com/D17217
Summary: Ref T5867. Use a single query to load both personal and global items, then reorder them and add a divider if both groups have some stuff.
Test Plan: Viewed menu, edited personal and global items, viewed/edited existing project menus.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17213
Summary:
Still lots to fix here, punting up since I'm running into a few roadblocks.
TODO:
[] Sort Personal/Global correctly
[] Quicksand in Help Items correctly on page changes
Test Plan: Verify new menus work on desktop, tablet, mobile. Test logged in menus, logged out menus. Logging out via a menu, verify each link works as expected. Help menus get build when using an app like Maniphest, Differential. Check that search works, preferences still save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12107
Differential Revision: https://secure.phabricator.com/D17209
Summary: Never really used this to full potential and takes up a lot of code and space. Remove option for now and make all profile nav menus small by default.
Test Plan: Review user, project, workboard. Set new menus.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17206
Summary:
Fixes T9430. Fixes T9362. Fixes T9544. This changes the default view of Audit to work like Differential, where commits you need to audit or respond to are shown in buckets.
This is a bit messy and probably needs some followups. This stuff has changed from a compatibility viewpoint:
- The query works differently now (but in a better, modern way), so existing saved queries will need to be updated.
- I've removed the counters from the home page instead of updating them, since they're going to get wiped out by ProfileMenu soon anyway.
- When bucketed queries return too many results (more than 1,000) we now show a warning about it. This isn't greaaaat but it seems good enough for now.
Test Plan: {F2351123}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9430, T9362, T9544
Differential Revision: https://secure.phabricator.com/D17192
Summary:
Fixes T6630. Long ago, "Audit", "Diffusion" and "Repositories" were three totally separate applications.
This separation isn't useful and the three rapidly became intertwined. Ideally, they would all be one application.
This doesn't take us quite that far, but Audit no longer has any controllers and has little actual behavior.
The "Audit" screen has always just been a SearchEngine view of commits with some filters on it, and this formalizes that and puts a link to it in Diffusion. (This view has other uses, too.)
Test Plan:
- Accessed audit from home page.
- Accessed audit/commits from Diffusion.
- Could no longer uninstall Audit on its own.
- Grepped for `/audit/` and `AuditApplication`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6630
Differential Revision: https://secure.phabricator.com/D17186
Summary: Ref T5867. Rough in a Favorites application, not wired to anything.
Test Plan: tbd. currently 404s so... I messed up something. Tossing up to read.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17160
Summary:
See D17160. Previously, the `/edit/` route was never linked, but fataled when accessed. Make it 404 instead.
Also, fix an issue where editing "Application" menu items would fail because they didn't have a viewer.
Test Plan:
- Hit `/edit/`, got a 404.
- Edited an "Application" item.
- Moved, added, deleted, and edited other items.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17165
Summary:
Ref T12074. The "v3" API methods (`*.search`, `*.edit`) are currently marked as "unstable", but they're pretty stable and essentially all new code should be using them.
Although these methods are seeing some changes, almost all changes are additive (support for new constraints or attachemnts) and do not break backward compatibility. We have no major, compatibility-breaking changes planned.
I don't want to mark the older methods "deprecated" yet since `arc` still uses a lot of them and there are some capabilities not yet available on the v3 methods, but introduce a new "frozen" status with pointers to the new methods.
Overall, this should gently push users toward the newer methods.
Test Plan: {F2325323}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17158
Summary: Ref T5867, adds a customPHID field, nullable, and lets you query by it... i think? Not fully able to grok all the EditEngine stuff, but I think this is the right place for the query.
Test Plan: Not wired to anything, but pulling up project menu, editing, all still works.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17149
Summary: Adds a FormEditEngine MenuItem for adding forms to Projects, Home, QuickCreate. Also adds an EditEngine typeahead that has token rendering issues currently.
Test Plan: Set a normal form as a menu item, edit it, set the name. Set a custom form as a menu item, edit it, set a name.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17098
Summary: Built similar to Projects, allows setting of a Dashboard to MenuItem.
Test Plan: Add a dashboard with and without a name / icon to a Project.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17092
Summary: Allows you to name and set a project as a menu item navigation element.
Test Plan: Add a project, no name, see project. Remove. Add a project and give it a short name (bugs) and see project link.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17021
Summary: Cleans up the UI on the page here, uses two column layout, places actions as actionlist instead of dropdown. Changes edit pages to dialogs.
Test Plan: Add an application, divider, link, and facts to a menu page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17030
Summary: Ref T11957.
Test Plan:
- Viewed an existing project profile.
- Viewed a user profile.
- Created a new project.
- Edited a profile menu.
- Added new profile items.
- Grepped for renamed symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17028
Summary:
Ref T11957. This renames the Configuration storage, transaction, query, and PHID type.
No rename on the actual menu item types yet, that's next (and should be the end of this, I think).
Test Plan:
- Viewed projects.
- Viewed profiles.
- Edited a project menu.
- Grepped for all renamed symbols, I think?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17027
Summary: Allows applications to be added as profile menu items
Test Plan: Add an application to a project, see menu item, click on menu. Uninstall application, see menu without application.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17016
Summary:
Fixes T11929. When running with a query, we no longer enforce an order on the subquery join to produce results more quickly when searching for common strings.
However, this means that empty queries (like those issued by "Close as Duplicate") don't order subquery results.
Restore a `dateCreated` order if there is no query text.
Test Plan: Artificially set limit to 10, still saw 10 most recent tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11929
Differential Revision: https://secure.phabricator.com/D16960
Summary: Found these in the `secure` error logs: one bad call, one bad column.
Test Plan: Searched for empty string. Double-checked method name.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16948
Summary:
Ref T6740. When we index a document, also save a copy of the stemmed version.
When querying, search the combined corpus for the terms.
(We may need to tune this a bit later since it's possible for literal, quoted terms to match in the stemmed section, but I think this wil rarely cause issues in practice.)
A downside here is that search sort of breaks if you upgrade into this and don't reindex. I wasn't able to find a way to issue the query that remained compatible with older indexes and didn't have awful performance, so my plan is:
- Put this on `secure`.
- Rebuild the index.
- If things look good after a couple of days, add a way that we can tell people they need to rebuild the search index with a setup warning.
We might get some reports between now and then, but if this is super awful we should know by the end of the weekend.
Test Plan:
WOW AMAZING
{F2021466}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6740
Differential Revision: https://secure.phabricator.com/D16947
Summary: Ref T11741. I'll wait until the release cut to land this; it just adds a test for InnoDB FULLTEXT being available instead of always returning `false`.
Test Plan:
- Ran with InnoDB fulltext locally for a day and a half without issues.
- Ran `bin/storage upgrade`, saw it detect InnoDB fulltext.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D16946
Summary:
Ref T6740. Currently, we issue fulltext queries with an "ORDER BY <score>" on the entire result set.
For very large result sets, this can require MySQL to do a lot of work. However, this work is generally useless: if you search for some common word like "diff" or "internet" or whatever and match 4,000 documents, the chance that we can score whatever thing you were thinking of at the top of the result set is nearly nothing. It's more useful to return quickly, and let the user see that they need to narrow their query to get useful results.
Instead of doing all that work, let MySQL find up to 1,000 results, then pick the best ones out of those.
This actual change is a little flimsy, since our index isn't really big enough to suffer indexing issues. However, searching for common terms on my local install (where I have some large repositories imported and indexed) drops from ~40ms to ~10ms.
My hope is to improve downstream performance for queries like "translatewiki" here, particularly:
<https://phabricator.wikimedia.org/T143863>
That query matches about 300 trillion documents but there's a ~0% chance that the one the user wants is at the top. It takes a couple of seconds to execute, for me. Better to return quickly and let the user refine their results.
I think this will also make some other changes related to stemming easier.
This also removes the "list users first" ordering on the query, which made performance more complicated and seems irrelevant now that we have the typeahead.
Test Plan:
- Searched for some common terms like "code" locally, saw similar results with better performance.
- Searched for useful queries (e.g., small result set), got identical results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6740
Differential Revision: https://secure.phabricator.com/D16944
Summary:
Ref T11741. This makes everything work if we switch to InnoDB, but never actually switches yet.
Since the default minimum word length (3) and stopword list (36 common English words) in InnoDB are generally pretty reasonable, I just didn't add any setup advice for them. I figure we're better off with simpler setup until we identify some real problem that the builtin stopwords create.
Test Plan: Swapped the `false` to `true`, ran `storage adjust`, got InnoDB fulltext indexes, searched for stuff, got default "AND" behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D16942
Summary:
Ref T11741. InnoDB uses a stopwords table instead of a stopwords file.
During `storage upgrade`, synchronize the table from the stopwords file on disk.
Test Plan:
- Ran `storage upgrade`.
- Ran `select * from stopwords`, saw stopwords.
- Added some garbage to the table.
- Ran `storage upgrade`, saw it remove it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D16940
Summary:
Ref T11741. Fixes T10642. Parse and compile user queries with a consistent ruleset, then submit queries to the backend using whatever ruleset MySQL is configured with.
This means that `ft_boolean_syntax` no longer needs to be configured (we'll just do the right thing in all cases).
This should improve behavior with RDS immediately (T10642), and allow us to improve behavior with InnoDB in the future (T11741).
Test Plan:
- Ran various queries in the UI, saw the expected results.
- Ran bad queries, got useful errors.
- Searched threads in Conpherence.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10642, T11741
Differential Revision: https://secure.phabricator.com/D16939
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary: Ref T11773. Not committed to this implementation, but adds some "Developer" query actions to jump to the nux/overheated states without needing to know secret magic URL variables.
Test Plan: {F1878984}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11773
Differential Revision: https://secure.phabricator.com/D16736
Summary: Ref T11773. This is an initial first step toward a more complete solution, but should make the worst case much less bad: prior to this change, the worst case was "30 second exeuction timeout". After this patch, the worst case is "no results + explanatory message", which is strictly better.
Test Plan:
Made all feed stories fail policy checks, loaded home page.
- Before adding overheating: 9,600 queries / 20 seconds
- After adding overheating: 376 queries / 800ms
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11773
Differential Revision: https://secure.phabricator.com/D16735
Summary:
Ref T10747. When a user drops a ".ics" file or a bunch of ".ics" files into a calendar view, import the events.
(Possibly we should just do this if you drop ".ics" files into any application, but we can look at that later.)
Test Plan: Dropped some .ics files into calendar views, got imports.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16722
Summary: Accept Conduit parameter values as strings (e.g. from `curl`) and convert to required type.
Test Plan:
Call conduit method with int/bool parameter iusing `curl` and make sure it does not result in validation error, e.g.
```
$ curl http://$PHABRICATOR_HOST/api/maniphest.search -d api.token=$CONDUIT_TOKEN -d constraints[modifiedEnd]=$(date +%s) -d constraints[hasParents]=true -d limit=1
```
Fixes T10456.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10456
Differential Revision: https://secure.phabricator.com/D16694
Summary: Background is now always white, spacing in header is more consistent
Test Plan: test mobile, table, desktop application search apps.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16691
Summary:
Ref T10747.
- Adds a "Use Results..." dropdown to query result pages, with actions you can take with search results (today: create export; in future: bulk edit, export as excel, make dashboard panel, etc).
- Allows you to create an export against a query key.
- I'm just using a text edit field for this for now.
- Fleshes out export modes. I plan to support: public (as though you were logged out), privileged (as though you were logged in) and availability (event times, but not details).
This does not actually export stuff yet.
Test Plan: Created some exports. Viewed and listed exports.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16676
Summary: Creates a background that renders inside the Quicksand frame, through sorcery.
Test Plan: Turn on Quicksand, visit lots of pages. See correct background colors. This probably blows something up I'm not testing.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16642
Summary:
Ref T11593. When you call a `*.search` method like `maniphest.search`, we don't currently validate that all the constraints you pass are recognized.
I think there were two very weak arguments for not doing this:
- It makes compatibility in `arc` across versions slightly easier: if we add a new constraint, we could add it to `arc` but also do client-side filtering for a while.
- Conduit parameter types //could//, in theory, accept multiple inputs or optional/alias inputs.
These reasons are pretty fluff and T11593 is a concrete issue caused by not validating. Just validate instead.
Test Plan:
- Made a `maniphest.search` call with a bogus constraint, got an explicit error about the bad constraint.
- Made a `maniphest.search` call with a valid constraint (`"ids"`).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11593
Differential Revision: https://secure.phabricator.com/D16507
Summary: Previously we collapsed all table search results, but the new UI doesn't need it. Remove unused methods and fix CSS.
Test Plan: Legalpad Signatures, Phortune Accounts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16469
Summary: Ref T11132, significantly cleans up the Config app, new layout, icons, spacing, etc. Some minor todos around re-designing "issues", mobile support, and maybe another pass at actual Group pages.
Test Plan: Visit and test every page in the config app, set new items, resolve setup issues, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16468
Summary: Ref T10951. This adds the application name as an attribute below the document type in the UI for doc type search.
Test Plan: Verify that the application name appears as an attribute on the document type results.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10951
Differential Revision: https://secure.phabricator.com/D16446
Summary: Ref T10951. This diff removes uninstalled applications from the result set for DocumentType restults
Test Plan: Uninstall an application (diviner for example), then go to the document type search menu and ensure that the uninstalled application doesn't show up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10951
Differential Revision: https://secure.phabricator.com/D16445
Summary: Added a brand new shiny cat fact
Test Plan: Pulled up a project with motivator installed and nothing broke
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16430
Summary: Switches over to new property UI boxes, splits core and apps into separate pages. Move Versions into "All Settings". I think there is some docs I likely need to update here as well.
Test Plan: Click on each item in the sidebar, see new headers.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16429
Summary:
Ref T11473. If you write a method like `get_stuff(ids)` and then call it with an empty list of IDs, you can end up passing an empty constraint to Conduit.
If you run a `*.search` method with such a constraint, like this one:
```
{
"ids": []
}
```
...we have three possible beahviors:
# Treat it like the user passed no constraint (basically, ignore the constraint).
# Respect the constraint (return no results).
# Error.
Currently, we do (1). However, this is pretty confusing and I think clearly the worst option, since it means `get_stuff(array())` in client code will often tend to return a ton of results.
We could do (2) instead, but this is also sort of confusing (it may not be obvious why nothing matched, even though it's an application bug) and I think most reasonable client code should be doing an `if ($ids)` test: this test makes clients a little more complicated, but they can save a network call, and I think they often need to do this test anyway (for example, to show the user a different message).
This implements (3), and just considers these to be errors: this is the least tricky behavior, it's consistent with what we do in PHP, makes fairly good sense, and the only cost for this is that client code may need to be slightly more complex, but this slightly more complex code is usually better code.
Test Plan: Ran Conduit `*.search` queries with `"ids":[]` and `"phids":[]`, got sensible errors instead of runaway result sets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11473
Differential Revision: https://secure.phabricator.com/D16396
Summary: I find this fact very useful for understanding my feline companion
Test Plan: Added "Motivator: Cat Facts" to the project, nothing broke
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16374
Summary: This moves aphront-side-nav to use same table css display as profile nav. Slightly less code to support. Cleans up AppSearch UI, think I've gotten all the edge cases here, but bang on it, can hold until after release cut.
Test Plan: Config, Maniphest, Differential, Diffusion, Home.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16346
Summary:
Ref T11404. Currently, SearchEngineAttachments can bulk-load data but SearchEngineExtensions can not.
This leads to poor performance of custom fields. See T11404 for discussion.
This changes the API to support a bulk load + format pattern like the one Attachments use. The next change will use it to bulk-load custom field data.
Test Plan:
- Ran `differential.query`, `differential.revision.search` as a sanity check.
- No behavioral changes are expected
- See next revision.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16350
Summary:
Ref T11326. This isn't perfect, but should be a little easier to use and less weird/confusing.
Generally, provide a "Query > Month > Day" crumb on day views, and a "Wed, July 3" header.
Generally, provide a "Query > Month" crumb on month views, and a "July 2019" header.
Also try to fix a bit of padding/spacing on the day view.
Test Plan: {F1739128}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16338
Summary:
Ref T11326. This doesn't go quite as far as the mock in T11326#185932, but gets rid of the easy margins.
Also cleans up some of the border rules so they're simpler and more consistent (no weird ragged edges on the far right).
Test Plan: {F1738951}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16335
Summary:
Fixes T11375. Some validation code was mishandling raw epoch timestamps.
For numeric values larger than 29999999 (e.g., 2999-12-25, christmas 2999), assume the value is a timestamp.
Test Plan: Used `maniphest.search` to query for `modifiedStart`, got a better result set and saw the `dateModified` constraint in the query.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11375
Differential Revision: https://secure.phabricator.com/D16326
Summary:
Ref T11326. These are last-generation and neither of these have callsites anymore.
(I nuked these since I'm trying to simplify date handling.)
Test Plan: Grepped for callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16301
Summary:
Fixes T11304. Prior to this change, we did an unnecessary write on every "*.search" call (this write didn't always actually write a row, since we only save //unique// saved queries, but still doesn't do anything useful ever, currently).
Instead, change this to not-write by default. We could add an "oh, and also I want you to do a write" option later, which would let us implement something like `arc query-stuff` which says "To see more results, view this URI in your browser: ...".
(It's possible to run one of these methods with an existing SavedQuery by using the key, so we still sometimes have a queryKey to return.)
Test Plan: Ran `almanac.service.search`, used DarkConsole to verify that no serachengine writes occurred.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11304
Differential Revision: https://secure.phabricator.com/D16263
Summary:
Fixes T11285. We can end up loading panel handles while processing edits (e.g., disabling menu items on a project). We probably started loading these after the modular transaction changes in T9789, which load the handle for the transaction object unconditionally.
The handles aren't too useful, but they currently fail to load/build because panels don't have a URI. We could give them some sort of method here, but just nuke it for now since they don't appear anywhere and this unclogs the daemon queue.
Test Plan:
- Disabled a menu item on a project.
- Ran publish task with `bin/worker execute --id <id>`.
- Before patch: fatal on getURI() with stack trace similar to T11285.
- After patch: clean execution.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11285
Differential Revision: https://secure.phabricator.com/D16249
Summary: Ref T4788. This seems reasonable locally, but not sure how it will feel on real data. Might need some tweaks, or might just be a terrible idea.
Test Plan: {F1708059}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16214
Summary:
Ref T4788. When closing a task as a duplicate of another task, you can only select one task, since it doesn't really make sense to merge one task into several other tasks (this operation is //possible//, but probably not what anyone ever wants to do, I think?).
Make the UI understand this: after you select a task, disable all of the "select" buttons in the UI to make this clear.
Test Plan:
- Used "Close as Duplicate", only allowed to select 1 task.
- Used other editors like "Merge Duplicates In", allowed to select lots of tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16203
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: 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 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
