Summary:
Fixes T12118. See PHI54. This adds a special case for the initial "reviewers" transactions, similar to the existing special case for "projects" transactions.
Although these transactions are redudnant in the web view since you can see the information clearly on the page, they're more reasonably useful in mail.
Test Plan: {F5168838}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12118
Differential Revision: https://secure.phabricator.com/D18542
Summary: Create a diff page, new UI
Test Plan: Create a diff from page
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18529
Summary: This simplifies EditEngine pages in general by removing the dual header, and extending to allow setting of a custom PHUIHeaderView if needed (like settings).
Test Plan:
Review all settings pages, review task, project pages. This should all be fine, but is a big change maybe some layouts I'm not considering. Tested these all mobile, destkop as well.
{F5166181}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18527
Summary: Updates settings panel UI for new white box, cleans up other various UI nitpicks.
Test Plan: Click through each setting that had a local setting page. Edit Engine pages will follow up on another diff.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18526
Summary: Adds a `MenuName` method to applications that `ProfileMenuItem` uses instead of the application name if set. This improves the home/menu/new user experience at little cost. Also renamed the label from Applications to Favorites, since this menu gets altered to provide more than just applications. This also allows instances to set back to Maniphest if they so choose. Overall I think this direction resolves 95% of my concerns, with maybe a small potential downside which I don't really anticipate. We already name Dashboard panels by their object, and that hasn't really caused confusion. I think these links are similar. I click 'Tasks' and get presented a list of my tasks from Maniphest.
Test Plan: Review each of the name changes as a default new install and a modified install.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18524
Summary: Simplifies the Repository Management pages to the new fixed column layout. I've also moved "Status" into the Basics page, which feels better, and moved "Documentation" as a nav item to a button in the header. This removed "action list" and "curtain view" from the management panels and uses the new bits from Config/Phacility. Undecided if the icons should stay or go for the nav. Left them in for Diffusion. I want to update the EditEngine pages to display in this UI and not leave the portal, but I haven't dug into that this page. I'm a bit worried it will not easily be possible.
Test Plan:
Generate a svn, git, hg repository, test each of the new pages and each of the new buttons. Activate, deactivate, etc.
{F5164674}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18523
Summary: Ref T12819. Adds storage and indexing for the Ferret engine to Differential.
Test Plan: Ran `bin/search index D123 --force`, saw indexes appear in database. No UI/user impact yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18540
Summary:
This is a full UI pass at a cleaner "Config" application. The main idea is to simplify the UI, center it, and have a different feel than other UI, a sort of "manage" UI theme for objects with loads of settings. Also adds a new minimalistic "WHITE_CONFIG" box type which may get re-used in Diffusion settings. This is a 90% pass, I'll have a few follow up diffs. Specifically:
- Build breadcrumbs as a flexible UI to go into headers.
- One click ObjectItemView option, for hover states.
- Sidenav doesn't always select (AphrontFilter issue)
- Mobile touchups, though it's pretty reasonable.
Test Plan:
Click through every page here, edit options, see new navigation UI. Test a few various setup issue layouts including fatals.
{F5163228}
{F5163229}
{F5163230}
{F5163231}
{F5163232}
{F5163233}
{F5163234}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18519
Summary: Ref T12819. Move these out of the core engine into the Ferret engine. In the future different applications can define different functions, like "summary:..." or whatever. This may get more formalization when I possibly do "author:" and such some time down the road.
Test Plan: Searched for "title:...". Searched for "dog:...", got a useful error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18536
Summary:
Ref T12819. Push more of the magic `' '` stuff into the engine and simplify calls to ngram construction.
Also fixes a bug where a task with title "apple banana" and description "cherry doughnut" could match query "banana cherry" by separating separate term segments with newlines instead of spaces.
Test Plan:
- Indexed some objects.
- Searched (term, substring, quoted terms).
- Viewed index in database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18534
Summary: Ref T12819. Earlier I separated some ngram code into an "ngram engine" hoping to share it across the simple Ngrams stuff and the full Ferret stuff, but they actually use slightly different rules. Just pull more of this stuff into FerretEngine to reduce the number of moving pieces and the amount of code duplication.
Test Plan: Searched for terms, rebuilt indexes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18533
Summary: Fixes T12975. This logic didn't deal with PolicyException correctly.
Test Plan: {F5167549}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12975
Differential Revision: https://secure.phabricator.com/D18537
Summary: See PHI46. The `core:` function means "find results in either the title or body, but not other auxiliary fields like comments".
Test Plan: Searched for text present in the title (yes), body (yes), and comments (no) with the `core:...` prefix.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18514
Summary:
Ref T12819. Some of the extensions "enrich" the document (adding more fields or relationships), while others "index" it (insert it into some kind of index for later searching).
Currently, these are all muddled under a single "index" phase. However, the Ferret extension cares about fields and relationships which other extensions may add.
Split this into two phases: "enrich" adds fields and relationships so other extensions can read them later if they want. "Index" happens after the document is built and has all the fields and relationships.
The specific problem this solves is that comments may not have been added to the document when the Ferret extension runs. By moving them to the "enrich" phase, the Ferret engine will be able to see and index comments.
Test Plan: Ran `bin/search index ...`, grepped for `indexFulltextDocument`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18513
Summary: Implements a new mobile view thats more fullscreen, not boxed, so more space. Fixes issues with mobile tables when scrolling overflowed content.
Test Plan: Test home, branch, tags, code, file browse, graph, compare, history, readme, open revisions, owners.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18505
Summary: Visually selects the button if blame is on.
Test Plan: Turn blame on and off in Diffusion on a file.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18504
Summary:
Ref T12819. Distinguishes between "term" queries and "substring" queries, and tries to match them correctly most of the time. For example:
- `example` matches "example", obviously.
- `~amp` matches "example", but `amp` does not.
- `examples` matches "example" through stemming.
- `"examples"` does not match "example" (quoted text does not stem).
- `"an examp"` does not match "an example" (quoted text is still term text).
- `~"an examp"` matches "an example" (quoted, substring-operator text uses substring search).
Test Plan: Ran searches similar to the above, they seemed to do what they should.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18500
Summary:
Ref T12819. Depends on D18492. Instead of passing a raw query into the Query layer, parse it first.
This allows the query layer to figure out which parts should be substring vs term match, and would allow the SearchEngine layer to do `author:...` eventually by picking it out before sending it to the Ferret engine.
Test Plan: Ran some Ferret queries. They work like before, except that nonsense like `-+"quack"` raises an exception now.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18499
Summary:
Ref T12819. Ferret currently does substring search, but this is not the default mode users expect: when you search for the "RICO" act, you do not expect to find documents containing "apRICOt" even though "RICO" is a substring.
To support term search, index the corpus as a list of terms with puncutation removed and whitespace normalized so the engine can match against it.
Test Plan:
Ran `storage upgrade`, ran `search index`, saw sensible database results:
```
rawCorpus: This is the task description.
Hark! Whom'st'dve eaten this "food" shall surely ~perish~?? #blessed
normalCorpus: thi the task descript hark whom dve eaten food shall sure perish bless
termCorpus: This is the task description Hark Whom'st'dve eaten this food shall surely perish blessed
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18498
Summary:
Ref T12819. This addresses two issues:
- One practical issue is that right now, if you search for "dog cat", and they appear in different fields (for example, "dog" appears ONLY in the title, while "cat" appears ONLY in a comment) we won't find the document. This is somewhat rare -- usually, if "dog" appears in the title, it's also repeated in the description -- but I think clearly a bug. To attack this, start automatically creating a virtual "ALL" field with the full document text which we'll use as the primary thing we match against.
- For fields which may occur more than once -- today, only comments -- aggregate them all into one big "all of the text" row instead of writing one row per comment. This partly addresses the first point ("dog" in one comment and "cat" in a different comment won't be found) and partly makes some of the query gymnastics easier.
Test Plan:
Ran `bin/storage upgrade`, ran `bin/search index <Txxx>`, saw sensible corpus values in the database:
```
mysql> select * from maniphest_task_ffield\G
*************************** 1. row ***************************
id: 3
documentID: 1981
fieldKey: full
rawCorpus: This is the task title
This is the task description.
normalCorpus: thi the task titl
thi the task descript
*************************** 2. row ***************************
id: 4
documentID: 1981
fieldKey: titl
rawCorpus: This is the task title
normalCorpus: thi the task titl
*************************** 3. row ***************************
id: 5
documentID: 1981
fieldKey: body
rawCorpus: This is the task description.
normalCorpus: thi the task descript
3 rows in set (0.00 sec)
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18497
Summary:
See PHI47. When users copy/paste a wall of text into a project tokenizer, we can end up performing a very large number of JOINs.
These JOINs seem okay locally and on `secure`, but the install in PHI47 reports hitting issues.
Since these queries are almost certainly illegitimate (I think no one uses 5+ words to find a project), just limit the search to the 5 longest tokens.
Note that typing 6 tokens will still almost always work, since the UI does additional filtering. However, if you have 100+ projects named "a b c d e ..." and search for "a b c d e z", you may not hit it. This is so degenerate that it's hard to imagine any users encountering it.
This is a stopgap fix, I'll file something longer-term as a followup.
Test Plan: Used `/typeahead/class/PhabricatorProjectDatasource/` to run queries. Saw the same results with shorter query plans for all reasonable queries.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18506
Summary: Only for grey buttons, but can expand. Sets a selected class.
Test Plan: Review new changes in UIExamples.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18501
Summary:
Ref T2543. The type on these got changed by accident, it should be "string" (crazy nonsense, compatible) not "int" (sensible, not compatible).
(New API uses sensible strings like "accepted" only.)
Test Plan: Called `differential.query` from web UI, saw `"2"` and similar statuses.
Reviewers: chad, jmeador, lvital
Reviewed By: jmeador, lvital
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18493
Summary:
Ref T2543. When called from the UI to build the dropdown, there's no Editor, since we aren't actually in an edit flow.
This logic worked for actually performing the edits, just not for getting the option into the dropdown.
Test Plan: Used the dropdown to close an "Accepted" revision which I authored.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18490
Summary: This removes the redundant "Account" label and item, and just keeps the page better aligned.
Test Plan: Review personal settings
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18489
Summary:
Ref T12819. Two minor improvements from live data:
- Tokenize in a UTF8-aware way.
- When one document fails to index, kill the transaction explicitly (rather than leaving it hanging) so we don't cause other failures later.
Test Plan: Created some UTF8 documents locally, indexed them, got clean results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18487
Summary: This panel just gets super tall at 15 now that date is on it's own line.
Test Plan: Reload panel, count to 10.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18486
Summary:
Ref T12819. I gave this stuff a sweet code name because all the terms related to "fulltext" and "search" already mean 5 different things. It, uh, ferrets out documents for you?
I'm building this to work a lot like the existing ngram index, which seems to work pretty well. If this sticks, it will auto-resolve the join issue (in T12443) by letting us do the entire thing locally in a JOIN and thus dodge a lot of mess.
This index gets built alongside other indexes, but only shows up in the UI if you have prototypes enabled. If you do, it appears under the existing fulltext field in Maniphest. No existing functionality is affected or disrupted.
NOTE: The query engine half of this is still EXTREMELY primitive, and this probably performs worse than the existing field for now. If this doesn't show obvious signs of being awful on `secure` I'll improve that in followup changes.
Test Plan:
Indexed my tasks, ran some simple queries, got the results I wanted, even for queries "ko", "k", "v0.1".
{F5147746}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819, T12443
Differential Revision: https://secure.phabricator.com/D18484
Summary: Fixes whatever task is tracking this junk, if one exists. Don't prompt unless there's a security issue.
Test Plan:
- Generated notifications from a test account.
- Clicked "Mark All" from dropdown menu, no prompt.
- Clicked "Mark All" from notifications screen, no prompt.
- Command-Clicked "Mark All" from dropdown menu to open in new window, got normal prompt.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18483
Summary: 50% more line, no additional cost! Order Now! Operators are standing by.
Test Plan: Blame a file
Reviewers: epriestley, avivey
Reviewed By: avivey
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18481
Summary: Just deletes the view code until I have time to better plan this out, or just not ship.
Test Plan: Visit Phame post on public logged out page, view count doesnt cause transaction fatal.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18475
Summary: These come out of the database as strings (see T12678), force them to integers for the API.
Test Plan: Called `transaction.search`, got integers in JSON instead of strings.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18476
Summary: My fake data was 100%, and not all tables have full revision history. This leads to a broken table. Instead check if we have //any// revisions at all, then always show the column, with or without a link inside.
Test Plan: going on a limb this is the correct fix and test on secure... again ...
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18474
Summary: There is still some layout issues with revisions, so I've tested it better and moved it to it's own column
Test Plan: Fake in some revision data, test various sizes and shapes.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18473
Summary:
Ref T5873. This provides paths and line numbers for inline comments.
This is a touch hacky but I was able to keep it mostly under control.
Test Plan:
- Made inline comments.
- Called API, got path/line information.
{F5120157}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873
Differential Revision: https://secure.phabricator.com/D18469
Summary: Minor cleanup, this logic can be simpler. Instead of special-casing inlines as having an effect if the have a comment, just consider any transaction with a comment to have an effect. I'm fairly certain this is always true.
Test Plan: Made inlines, tried to submit empty comments. Behavior unchanged.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18468
Summary:
Ref T5873. See PHI14. I don't want to just expose internal transaction data to Conduit by default, since it's often: unstable, unusable, sensitive, or some combination of the three.
Instead, let ModularTransactions opt in to providing additional data to Conduit, similar to other infrastructure. If a transaction doesn't, the API returns an empty skeleton for it. This is generally fine since most transactions have no real use cases, and I think we can fill them in as we go.
This also probably builds toward T5726, which would likely use the same format, and perhaps simply not publish stuff which did not opt in.
This doesn't actually cover "comment" or "inline comment", which are presumably what PHI14 is after, since neither is modular. I'll probably just put a hack in place for this until they can modularize since I suspect modularizing them here is difficult.
Test Plan: Ran `transaction.search` on a revision, saw some transactions (title and status transactions) populate with values.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873
Differential Revision: https://secure.phabricator.com/D18467
Summary:
Ref T5873. See PHI14. This does the basics that are shared across everything (IDs, PHIDs, dates, comments).
It doesn't do types (I think I don't necessarily want to expose internal types over the API?) or transaction-specific data.
In the next change, I'm going to add ways to let ModularTransactions "opt-in" to providing more data to Conduit. I'll use this to flesh out the actual desired transaction types (comments, presumably inline comments) and likely leave the rest as skeletons for now until use cases arise so we don't create a backward compatibility issue (or a security issue!) by exposing tons of internal stuff as public-facing API.
Test Plan:
Ran queries, used paging. Retrieved an edited, deleted, and normal comment.
{F5120060}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873
Differential Revision: https://secure.phabricator.com/D18466
Summary:
Fixes T12970. This is easier than I expected, and appears to occur in only one place.
This prevents a change from ever generating with an anchor like `#12345678`, which is ambiguous because it may be a comment anchor.
Test Plan: Viewed a revision, saw new `change-xxxyyyzzz` anchors, clicked one, got jumped to the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12970
Differential Revision: https://secure.phabricator.com/D18465
Summary:
Ref T12956. UI changes:
- Administrators get a new `[X] Save as global query` option when saving a query.
- "Edit Queries..." is split into "Personal" and "Global" sections. For administrators, each section can be edited. For non-admins, only the top section can be edited, but any query can be pinned.
A couple notes:
- This doesn't support "pin for everyone by default". New users just get the first query from the bottom set. That seems reasonable for now.
- Reordering is currently a little buggy (it works if you've reordered before, but not if you're reordering for the first time), but I need to migrate before I can fix / test that properly. So that'll get cleaned up in the next change or two.
Test Plan:
- As an admin and non-admin, viewed, edited, disabled, saved-as-personal and saved-as-global various queries.
{F5098581}
{F5098582}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12956
Differential Revision: https://secure.phabricator.com/D18426
Summary:
Ref T12956. Currently, when you visit `/maniphest/` (or any other ApplicationSearch application) we execute the first query in the list by default.
In T12956, I plan to make changes so that personal queries are always first, then global/builtin queries. Without changing the "default query" rule, this will make it harder to have, for example, some custom queries in Differential but still run a global query like "Active" by default. To make this work, you'd have to save a personal copy of the "Active" query, then put it at the top.
This feels a bit cumbersome and this rule is kind of implicit and a little weird anyway. To make this work a little better as we make changes here, add an explicit pinning action, like the one we have in Project ProfileMenus.
You can now explicitly choose a query to make default.
Test Plan:
- Browsed without pinning anything, saw normal behavior.
- Pinned queries, viewed `/maniphest/`, saw a non-initial query selected by default.
- Pinned a query, deleted it, nothing exploded.
{F5098484}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12956
Differential Revision: https://secure.phabricator.com/D18422
Summary:
See PHI42. Currently, `maniphest.search` incorrectly applies this default (group by priority) to all queries via Conduit.
The correct behavior is to apply no grouping constraint.
I think this is also a reasonable general behavior, and the current code seems to date from D6960 in 2013 and didn't seem particularly carefully considered.
This is a minor compatibility break -- saved queries which are more than 4 years old might change their group behavior. I'll note this in the change logs but expect essentially no one to be affected.
Test Plan: Ran a `maniphest.search` Conduit call and observed the underlying query. Before this change, it executed `ORDER BY priority, id`. After this change, it correctly executed `ORDER BY id` only.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18459
Summary: Using icons and dropdown buttons without text looks a little wonky, this resets the CSS a bit.
Test Plan: Review button with icon and text, just icon, just test, and dropdowns.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18461
Summary:
See PHI39. This adds support for editing parents and subtasks of a task via Conduit.
It might be nice to tie this into the `PhabricatorObjectRelationship` stuff eventually, but I think we'd effectively end up in the same place anyway in terms of what the API looks like.
Test Plan: {F5116163}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18456
Summary: Fixes T12792. Expands the Notifications to "web, desktop, both, or none" for real-time notifications in settings.
Test Plan: Test with "test notifications" button, and while logged into two accounts with each of the 4 settings.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12792
Differential Revision: https://secure.phabricator.com/D18457
Summary: I missed an anchor tag here, adds it back
Test Plan: View blame, click a previous version of the file, click Back to HEAD link.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18451
Summary: Ref T12824, adds more information to the blame view, exposes date, commit summary, lighter colors.
Test Plan:
Review many diffs with and without blame on.
{F5111758}
{F5111759}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18452
Summary: Fixes T12969. If you disable "Home" but leave it at the top, we still load it.
Test Plan: Disabled "Home". Move Dashboard into first position, see correct home layout.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12969
Differential Revision: https://secure.phabricator.com/D18455
Summary:
See PHI36. APCu originally had `apc_` methods, but at some point dropped these and only provides `apcu_` methods.
When the `apcu_` method is present, use it. It may not be present for older versions of APCu, so keep the fallback.
Test Plan:
- With modern APCu, clicked "Purge Caches" in Config > Caches.
- Before: fatal on bad `apc_clear_caches` call.
- After: Valid cache clear.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18449
Summary: This adds a very very basic view count to Phame, so bloggers can get some idea which posts are more popular than others. Anything more than this I think should be Facts or Google Analytics.
Test Plan: Write a new post, see post count. Reload page, post count goes up. Archive post, post count stays the same.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18446
Summary: Ref T12964. This feels like a cheat, but works well. Just redirect the user back to the form they came from instead of to the key page.
Test Plan: Add a key to a user profile, add a key to an Alamanac device. Grep for PhabricatorAuthSSHKeyTableView and check all locations.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12964
Differential Revision: https://secure.phabricator.com/D18445
Summary: Moves browseFile to single column, implements Owners as a list under the file (and now directory as well), improved information listed in Owners, and moves actions into the Diffusion action bar instead of the header.
Test Plan:
Test browsing directories, files, text, images, binaries, enabling blame. Mobile and desktop.
{F5111045}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18448
Summary: Adds some basic UI for open / closed state when viewing a list of branches in Mercurial. Fixes T12838
Test Plan: Close and open branches, view list.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12838
Differential Revision: https://secure.phabricator.com/D18447
Summary: Better table layouts here for branches view
Test Plan: Test git, hg repositories. See column go away.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18444
Summary: This is in the crumbs, but a little hidden. Puts branch name at the top of the browse table header.
Test Plan: Review a few branchs, change branch, see new name.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18441
Summary: Adds an icon for default branch, status for branch status
Test Plan: Review `hg` and `git` repositories, change default branch, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18443
Summary:
Ref T12966. See that task for a description and reproduction steps.
If you put Phabricator in a master/replica configuration and then restart it, we may fatal here if the master is unreachable. Instead, we should survive setup checks.
Test Plan: Put Phabricator in a master/replica configuration, explicitly disabled the master by misconfiguring the port, restarted Phabricator. Before: fatal; after: login screen in read-only mode.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12966
Differential Revision: https://secure.phabricator.com/D18442
Summary:
Ref T12965. See that task for discussion, and PHI36 for context.
This sweeps the fatal under the rug by skipping it, letting things move forward for now.
Test Plan: Followed instructions in T12965, got a read-only recovery after restart instead of a fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12965
Differential Revision: https://secure.phabricator.com/D18440
Summary: Simplifies the page, adds base support for PHUITwoColumn fixed from Instances (which I'll delete css there).
Test Plan:
click on every settings page, UI seems in tact, check mobile, desktop, mobile menus.
{F5102572}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18436
Summary: Moves the method up to DiffusionController, so it can be more universally used. Also now center aligns tabs on mobile. Still todo, get search nicely toggled on mobile
Test Plan: Test mobile, desktop. Test search from home, from browse, and browsing a specific path.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18432
Summary: Moving this down the the "bar" to allow pattern search on home. Rebuilds the mobile layout a little.
Test Plan:
Test actions on mobile, desktop, tablet.
{F5100460}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18431
Summary: Roughs this in a little, kinda basic. Allows for grouping results by page. A bit better on mobile. Would like more content return from conduit though.
Test Plan:
Test `CMS`, `cms`, and `OMGLOLWTFBBQ`, desktop and mobile
{F5099081}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18429
Summary: This is only on browse pages, but I think could be global (home) also. Moves it from a button, field, to just a field.
Test Plan:
Review search on desktop, mobile.
{F5098886}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18428
Summary: Removing this cleanly in event we want to put it back later. 99% of these cases are likely workable either by command line or the typeahead. Will gauge feedback if users notice.
Test Plan: Reload page, perform file grep search.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18425
Summary: Getting to the straight browse view went away, this adds a link back. I'll look at more long term solution for getting to grep search.
Test Plan: Click on header, get take to browse view.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18421
Summary: Ref T12956. No real behavioral changes here, just slightly more modern code.
Test Plan: Reviewed named queries in Maniphest and "Edit Queries...".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12956
Differential Revision: https://secure.phabricator.com/D18420
Summary:
Ref T2543. This updates and migrates the status change transactions:
- All storage now records the modern modular transaction ("differential.revision.status"), not the obsolete non-modular transaction ("differential:status").
- All storage now records the modern constants ("accepted"), not the obsolete numeric values ("2").
Test Plan:
- Selected all the relevant rows before/after migration, data looked sane.
- Browsed around, reviewed timelines, no changes after migration.
- Changed revision states, saw appropriate new transactions in the database and timeline rendering.
- Grepped for `differential:status`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18419
Summary:
Ref T2543. Rewrites all the storage to use constants.
Note that transactions still use legacy values, I'll migrate and update them separately.
Test Plan:
- Ran migration.
- Browsed around, changed revision states, viewed dashboard, etc.
- Selected `DISTINCT()` and `GROUP_CONCAT()` of the `status` field in the database, saw sane/expected before and after values.
- Verified that old Conduit methods still return numeric constants for compatibility.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18418
Summary: Ref T2543. All writers now write modern statuses. Make all readers explicit about whether they are reading modern or legacy statuses, so I can swap the storage format.
Test Plan:
- Grepped for `getStatus()`, scanned the list. Other applications have methods with this name so it's possible I missed something.
- Browed around, changed revision statuses.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18417
Summary:
Ref T2543. These are the last `ArcanistDifferentialRevisionStatus` callsites.
This removes the very old legacy `precommitRevisionStatus` field, which has no other readers. This was obsoleted by the `CLOSED_FROM_ACCEPTED` stuff, but retained for compatibility.
Test Plan:
- Poked these with the test console, although they're a little tricky to be sure about.
- Grepped for `ArcanistDifferentialRevisionStatus`, no more hits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18416
Summary: Ref T2543. Update these for the modern stuff.
Test Plan: Created a new revision, got a revision in the right state ("Needs Review"). Accepted, planned, requested, abandoned revision; state transitions looked good.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18415
Summary: Ref T2543. Swaps these over to modern constants.
Test Plan: Viewed dashboard, no chagnes to bucketing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18414
Summary: Ref T2543. This cleans up all the "when no one is rejecting/blocking and someone accepted, mark the revision overall as accepted" logic to use more modern status stuff instead of `ArcanistDifferentialRevisionStatus`.
Test Plan:
- Updated revisions, saw them go to "Needs Review".
- Accepted, requested changes to revisions.
- Updated one with changes requested, saw it go to "needs review" again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18413
Summary:
Ref T2543. This cleans up a couple of remaining rough edges:
- We could do an older TYPE_ACTION "close" via the daemons.
- We could do an older TYPE_ACTION "close" via `arc close-revision`, explicitly or implicitly in `arc land`, via API (`differential.close`).
- We could do an older TYPE_ACTION "rethink" ("Plan Changes") via the API, via `arc diff --plan-changes` (`differential.createcomment`).
Move these to modern modular transactions, then get rid of all the validation and application logic for them. This nukes a bunch of `ArcanistDifferentialRevision::...` junk.
Test Plan:
- Used `bin/repository reparse --message rXYZ...` to reparse a commit, closing a corresponding revision.
- Used `differential.close` to close a revision.
- Used `differential.createcomment` to plan changes to a revision.
- Reviewed transaction log for full "closed by commit" message (linking to commit and mentioning author).
- Grepped for `::TYPE_ACTION` to look for remaining callsites, didn't find any.
- Grepped for `differential.close` and `differential.createcomment` in `arcanist/` to look for anything suspicious, seemed clean.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18412
Summary:
Ref T2543. Converts the TYPE_STATUS transaction (used to render "This revision now requires changes to proceed.", "This revision is accepted and ready to land.", etc) to ModularTransactions.
Also, continue consolidating all the status-related information (here, more colors and icons) into a single place. By the end of this, we may learn that NEEDS_REVIEW uses //every// color.
Test Plan:
Reviewed old status transactions (unchanged) and created new ones (looked the same as the old ones).
(I plan to migrate all of these a few diffs from now, around when I change the storage format.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18410
Summary: Ref T2543. Cleans up some more references to ArcanistDifferentialRevisionStatus, moving toward getting rid of it completely.
Test Plan: Planned changes, requested review, inspected the "close" one since it isn't trivial to trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18408
Summary: Ref T2543. Now that the integer status constants are banished to the internals, we can expose status information from "differential.revision.search".
Test Plan:
Searched for revisions.
{F5093873}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18400
Summary: Ref T2543. I converted this condition the wrong way, missing a `!`. I'll cherry-pick this to `stable`.
Test Plan: No more "Reopen Revision" action available on open revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18399
Summary:
Ref T2543. Ref T10967. This isn't precisely related to "draft" status, but while I'm churning this stuff anyway, get rid of the old double writes to clean the code up a bit.
These were added in T10967 to make sure the migration was reversible/recoverable, but we haven't seen any issues with it in several months so I believe they can now be removed safely. Nothing has read this table since ~April.
Test Plan: Took various review actions on revisions (accept, reject, resign, comment, etc). If this change is correct, there should be no visible effect.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967, T2543
Differential Revision: https://secure.phabricator.com/D18398
Summary:
Ref T2543. I believe there have been no upstream callsites of this method since D1646, in February 2012.
The method works, and we can revert this if needbe, but this seems like a good time to remove support.
Test Plan: Grepped for `differential.find`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18397
Summary: Ref T2543. All callsites are now in terms of `withStatuses()`.
Test Plan:
- Called `differential.query` and `differential.find` from Conduit API.
- Grepped through all `withStatus()` callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18396
Summary: Ref T2543. Several queries want only open revisions. Provide a tailored, non-legacy way to issue that query.
Test Plan: Viewed some of these callsites (e.g., "Similar open revisions affecting these files"), saw only open revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18395
Summary:
Ref T2543. This updates the UI control in the web UI. Also:
- This implicitly makes this queryable with the API (`differential.revision.search`); it previously was not.
- This does NOT migrate existing saved queries. I'll do those in the next change, and hold this until it happens.
- This will break some existing `/differential/?status=XYZ` links. For example, `status=open` now needs to be `status=open()`. I couldn't find any of these in the upstream, and I suspect these are rare in the wild (users would normally link directly to saved queries, not use URI query construction).
Test Plan: {F5093611}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18393
Summary:
Ref T2543. This adds a tokenizer, similar to the Maniphest tokenizer, so the hard-coded `<select />` control in Differential ApplicationSearch can be replaced with a more flexible control that handles the addition of new statuses with more grace.
This only adds the new datasource.
Test Plan: Used `/typeahead/class/` to preview the behavior of the new datasource.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18392
Summary:
Ref T12961. Fixes T4416. Currently, for observed Mercurial repositories, we build a working copy with `pull -u` (for "update").
This should be unnecessary, and we don't do it for hosted Mercurial repositories. We also stopped doing it years ago for Git repositories. We also don't clone Mercurial repositories with a working copy.
It's possible something has slipped through the cracks here so I'll hold this until after the release cut, but I believe there are no actual technical blockers here.
Test Plan:
- Observed a public Mercurial repository on Bitbucket.
- Let it import.
- Browsed commits, branches, file content, etc., without any apparent issues.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12961, T4416
Differential Revision: https://secure.phabricator.com/D18390
Summary:
Ref T12961. In Mercurial, it's possible to have "subrepos" which may use a different protocol than the main repository.
By putting an SSH repository inside an HTTP repository, an attacker can theoretically get us to execute `hg` without overriding `ui.ssh`, then execute code via the SSH hostname attack.
As an immediate mitigation to this attack, specify `ui.ssh` unconditionally. Normally, this will have no effect (it will just be ignored). In the specific case of an SSH repo inside an HTTP repo, it will defuse the `ssh` protocol.
For good measure and consistency, do the same for Subversion and Git. However, we don't normally maintain working copies for either Subversion or Git so it's unlikely that similar attacks exist there.
Test Plan:
- Put an SSH subrepo with an attack URI inside an HTTP outer repo in Mercurial.
- Ran `hg up` with and without `ui.ssh` specified.
- Got dangerous badness without `ui.ssh` and safe `ssh` subprocesses with `ui.ssh`.
I'm not yet able to confirm that `hg pull -u -- <uri>` can actually trigger this, but this can't hurt and our SSH wrapper is safer than the native behavior for all Subversion, Git and Mercurial versions released prior to today.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12961
Differential Revision: https://secure.phabricator.com/D18389
Summary: Fixes T12832. Adds a basic table (not paginated?) to view tracking and autoclose status.
Test Plan:
Review a large repository (Krita) with setting various states of tracking and autoclose.
{F5092117}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12832
Differential Revision: https://secure.phabricator.com/D18386
Summary: See PHI31. The "Accepted Older Revision" icon is (more reasonably) bluegrey, but that rule spilled over here where it doesn't make much sense. "Requested Changes to Prior Diff" remains in effect across updates, but the coloration implies otherwise.
Test Plan:
"Requested Changes to This Diff" (unchanged):
{F5092019}
"Requested Changes to Prior Diff" (now red, previously bluegrey):
{F5092020}
Note that the icons are different so this is technically colorblind-safe, and it's normally not important to distinguish between these two reds anyway.
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Differential Revision: https://secure.phabricator.com/D18385
Summary:
Fixes T12948. See that task for substantial discussion and context. Briefly:
- This workflow is very old, and won't work for large (>2GB) files.
- This workflow has become more dangerous than it once was, and can fail in several ways that delete data and/or make recovery much more difficult (see T12948 for more discussion).
- This was originally added in D6068, which is a bit muddled, but looks like "one install ran into a weird issue so I wrote a script for them"; this would be a Consulting/Support issue and not come upstream today. I can't identify any arguments for retaining this workflow there, at least.
Test Plan:
- Grepped for `files purge`, got nothing.
- Grepped for `purge`, looked for anything that looked like instructions or documentation, got nothing.
I don't recall recommending anyone run this script in many years, and didn't even remember that it existed or what it did when T12948 was reported, so I believe it is not in widespread use.
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence
Maniphest Tasks: T12948
Differential Revision: https://secure.phabricator.com/D18384
