Summary:
Ref T13043. Currently:
- Passwords are stored separately in the "VCS Passwords" and "User" tables and don't share as much code as they could.
- Because User objects are all over the place in the code, password hashes are all over the place too (i.e., often somewhere in process memory). This is a very low-severity, theoretical sort of issue, but it could make leaving a stray `var_dump()` in the code somewhere a lot more dangerous than it otherwise is. Even if we never do this, third-party developers might. So it "feels nice" to imagine separating this data into a different table that we rarely load.
- Passwords can not be //revoked//. They can be //deleted//, but users can set the same password again. If you believe or suspect that a password may have been compromised, you might reasonably prefer to revoke it and force the user to select a //different// password.
This change prepares to remedy these issues by adding a new, more modern dedicated password storage table which supports storing multiple password types (account vs VCS), gives passwords real PHIDs and transactions, supports DestructionEngine, supports revocation, and supports `bin/auth revoke`.
It doesn't actually make anything use this new table yet. Future changes will migrate VCS passwords and account passwords to this table.
(This also gives third party applications a reasonable place to store password hashes in a consistent way if they have some need for it.)
Test Plan: Added some basic unit tests to cover general behavior. This is just skeleton code for now and will get more thorough testing when applications move.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18894
Summary:
See PHI173. Currently, Herald has an "Assign to" action for tasks, and you can specify custom fields with datasource values (like users or projects) that have a limit (like 1 "Owner", or 12 "Jury Members").
Herald doesn't support these limits right now, so you can write `[ Assign to ][ X, Y, Z ]`. This just means "Assign to X", but make it more clear by actually enforcing the limit in the UI.
Test Plan:
- Created a "projects" custom field with limit 1.
- Tried to create actions that 'assign to' or 'set custom field to' more than one thing, got helpfully rebuffed by the UI.
- Created an "add subscribers" action with more than one value.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18887
Summary:
Fixes T13042. This hooks up the new "silent" mode from D18882 and makes it actually work.
The UI (where we tell you to go run some command and then reload the page) is pretty clumsy, but should solve some problems for now and can be cleaned up eventually. The actual mechanics (timeline aggregation, Herald interaction, etc.) are on firmer ground.
Test Plan:
- Made a normal bulk edit, got mail and feed stories.
- Made a silent bulk edit, no mail and no feed.
- Saw "Silent Edit" marker in timeline for silent edits:
{F5386245}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13042
Differential Revision: https://secure.phabricator.com/D18883
Summary: Ref T13025. We're getting kind of a lot of actions, so put them in nice groups so they're easier to work with.
Test Plan: {F5386038}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18880
Summary:
Depends on D18866. Ref T13025. Fixes T12415. This makes the old "Add Comment" action work, and adds support for a new "Set description to" action (possibly, I could imagine "append description" being useful some day, maybe).
The implementation is just a `<textarea />`, not a whole fancy remarkup box with `[Bold] [Italic] ...` buttons, preview, typeaheads, etc. It would be nice to enrich this eventually but doing the rendering in pure JS is currently very involved.
This requires a little bit of gymnastics to get the transaction populated properly, and adds some extra validation since we need some code there anyway.
Test Plan:
- Changed the description of a task via bulk editor.
- Added a comment to a task via bulk editor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T12415
Differential Revision: https://secure.phabricator.com/D18867
Summary:
Depends on D18862. See PHI173. Ref T13025. Fixes T10005. This redefines bulk edits in terms of EditEngine fields, rather than hard-coding the whole thing.
Only text fields -- and, specifically, only the "Title" field -- are supported after this change. Followup changes will add more bulk edit parameter types and broader field support.
However, the title field now works without any Maniphest-specific code, outside of the small amount of binding code in the `ManiphestBulkEditor` subclass.
Test Plan: Used the bulk edit workflow to change the titles of tasks.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T10005
Differential Revision: https://secure.phabricator.com/D18863
Summary:
Depends on D18805. Ref T13025. Fixes T10268.
Instead of using a list of IDs for the bulk editor, power it with SearchEngine queries. This gives us the full power of SearchEngine and lets us use a query key instead of a list of 20,000 IDs to avoid issues with URL lengths.
Also, split it into a base `BulkEngine` and per-application subclasses. This moves us toward T10005 and universal support for bulk operations.
Also:
- Renames most of "batch" to "bulk": we're curently inconsitent about this, I like "bulk" better since I think it's more clear if you don't regularly interact with `.bat` files, and newer stuff mostly uses "bulk".
- When objects in the result set can't be edited because you don't have permission, show the status more clearly.
This probably breaks some stuff a bit since I refactored so heavily, but it seems mostly OK from poking around. I'll clean up anything I missed in followups to deal with remaining items on T13025.
Test Plan:
{F5302300}
- Bulk edited from Maniphest.
- Bulk edited from a workboard (no more giant `?ids=....` in the URL).
- Hit most of the error conditions, I think?
- Clicked the "Cancel" button.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T10268
Differential Revision: https://secure.phabricator.com/D18806
Summary:
Ref T13025. See PHI50. Fixes T11286. Ref T10005. Begin modernizing the bulk editor.
For T10005 ("move the bulk editor to modern infrastructure"), rewrite the rendering of the editable set so that it is application-agnostic and can work with any kind of object.
For T11286 ("let users de-select items in the working set"), make the working set editable.
Test Plan:
{F5302158}
- Deselected some objects, applied an edit, saw the edit apply to only selected objects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T11286, T10005
Differential Revision: https://secure.phabricator.com/D18805
Summary:
See PHI242. All use cases for this that I know of are pretty hacky, but they don't seem perilous, and it's easier than webhooks.
See P1895, T10183, and T9853 for me previously refusing to implement this since all those use cases were also pretty bad.
Test Plan:
- Wrote a rule to add comments, saw it add comments.
- Reviewed summary, re-edited rule, reviewed transcript to check that all the strings worked OK.
- Wrote a new rule for a non-commentable object (a blog) to make sure I wasn't offered the "Add a comment" action.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18823
Summary:
Ref T10233. See PHI231. When users ignore the `arc land` prompt about bad revision states, make it explicitly clear in the transaction log that they broke the rules.
You can currently figure this out by noticing that there's no "This revision is accepted and ready to land." message, but it's unrealistic to expect non-expert users to look for the //absence// of a message to indicate something, and this state change is often relevant.
Test Plan: {F5302351}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10233
Differential Revision: https://secure.phabricator.com/D18808
Summary:
Ref T13018. See that task and the Discourse thread for discussion.
This doesn't work as-is and we need to `og:description` everything to make it work. I don't want to sink any more time into this so just back all the changes out for now.
(The `<html>` change is unnecessary anyway.)
Test Plan: Strict revert.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13018
Differential Revision: https://secure.phabricator.com/D18782
Summary:
See PHI180. Currently, if you begin creating or editing an inline and then swap display modes (for example, with "View Unified"), your edit is lost.
Persisting the editor state is complicated and this is very rare, so just prevent the action and warn the user instead.
Also make the warning persist for a little longer since a few of the messages, including this one, take a couple seconds to read now.
Test Plan:
- Edited a comment, tried to swap display modes, got a warning.
- Swapped display modes normally with no comment being edited.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18774
Summary:
Ref T12539. See PHI190. Currently, each Diff has a `revisionID`, but Revisions do not point at the current active diff. To find the active diff for a given revision, we need to issue a separate query.
Furthermore, this query is inefficient for bulk loads: if we have a lot of revisions, we end up querying for all diff IDs for all those revisions first, then selecting the largest ones and querying again to get the actual diff objects. This strategy could likely be optimized but the query is a mess in any case.
In several cases, it's useful to have the active diff PHID without needing to do a second query -- sometimes for convenience, and sometimes for performance.
T12539 is an example of such a case: it would be nice to refine the bucketing logic (which only depends on active diff PHIDs), but it feels bad to make the page heavier to do it.
For now, this is unused. I'll start using it to fix the bucketing issue, and then we can expand it gradually to address other performance/convenience issues.
Test Plan:
- Ran migrations, inspected database, saw sensible values.
- Created a new revision, saw a sensible database value.
- Updated an existing revision, saw database update properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12539
Differential Revision: https://secure.phabricator.com/D18756
Summary:
See PHI185. When looking at a revision, you can Command-Click (Mac) symbols to jump to their definitions (provided the symbol index has been built).
Currently, the code works on any node inside the changeset list, so it activates when clicking links inside inline comments and opening them in a new window.
To avoid this, don't activate if we're inside an inline comment. This technically prevents you from doing a symbol lookup on a symbol inside a codeblock inside an inline, but that seems fine/reasonable.
Test Plan: Wrote `Dxxx` in an inline, command-clicked it. Before: got a symbol lookup. After: just a new tab with the revision.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18753
Summary: Give profile images a little more space, fix "/" spacing, add a tooltip.
Test Plan: {F5251205}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18749
Summary: Depends on D18746. See PHI174. Adds small author portraits next to each blame line (this is similar to GitHub).
Test Plan:
My local test data isn't that great since I don't have commits from a lot of accounts, but looks functional:
{F5251056}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18747
Summary:
Ref PHI174. This reverts most of these changes:
- 37843127e9 / D18481
- 94cad30ac3 / D18474
- 12ae08b6b1 / D18473
- 0a01334172 / D18462
- ac91ab1ef9 / D18452
These changes made the Diffusion blame view very similar to GitHub's blame view. See D18452 for a before/after of the bulk of these changes; the other revisions are bugfixes.
I think this was generally a step backward, and not motivated by solving a specific problem. I've found the new UI less usable than the old one, and at least one install (see PHI174) also has.
In particular, the revision/commit titles are very bulky and not terribly useful; the date column also isn't terribly useful; the "age" color actually IS pretty useful and was heavily de-emphasized.
I've kept one bugfix here (missing `'a'` tag type) and kept the upgraded icon for "Skip Past This Commit".
I'm going to follow this up with some additional changes:
- Show a small author profile icon, similar to GitHub, to address PHI174 more directly.
- Try a zebra-stripe on blocks of rows to make it more clear where changes affected by a particular commit begin and end.
- Try a hue shift, not just a brightness/saturation shift, to make the "age" color more distinct.
- Try computing colors as even steps, not based purely on age. Currently, if a file has one long-distant commit and several recent commits, all the recent ones show up as very bright green. I think this would probably be more useful if they were distributed more evenly across the available color bands.
Test Plan:
Viewed blame views in Diffusion, saw a more compact UI similar to the old UI.
{F5251019}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18746
Summary: See PHI177. Ref T12974. PonderQuestion was overlooked during the Ferret engine conversions.
Test Plan:
Ran migrations, searched for questions, got results:
{F5241185}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12974
Differential Revision: https://secure.phabricator.com/D18736
Summary:
Ref T13000. This adds support for tracking "common" ngrams, which occur in too many documents to be useful as part of the ngram index.
If an ngram is listed in the "common" table, it won't be written when indexing documents, or queried for when searching for them.
In this change, nothing actually writes to the "common" table. I'll start writing to the table in a followup change.
Specifically, I plan to do this:
- A new GC process updates the "common" table periodically, by writing ngrams which appear in more than X% of documents to it, for some value of X, if there are at least a minimum number of documents (maybe like 4,000).
- A new GC process deletes ngrams that have been added to the common table from the existing indexes.
Hopefully, this will pare down the ngrams index to something reasonable over time without requiring any manual tuning.
Test Plan:
- Ran some queries and indexes.
- Manually inserted ngrams `xxx` and `yyy` into the ngrams table, searched and indexed, saw them ignored as viable ngrams for search/index.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18672
Summary: Hide navbar, and make curtain behave like on a phone, when printing.
Test Plan: {F5197340}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18583
Summary:
Ref T12987. I was focused on the RefCursor table and overlooked that we need some care on this key.
It's currently possible to run `bin/storage upgrade --no-adjust`, then start Phabricator, and end up with duplicate records in this table. If you try to run `bin/storage adjust` later, it will try to add the unique key but fail. This is unusual for normal installs (they usually do not use `--no-adjust`) but we do it in the cluster and I did this exact thing on `secure`.
Normally, to avoid this, when a new table with a unique key is introduced, we also add a migration to explicitly add that key.
This is mostly harmless in this case. Fix this mistake (force the table to contain only unique rows; add the key) and try using `LOCK TABLES` to make this atomic. If this doesn't cause problems we can use this in similar situations in the future.
The "alter table may unlock things" warning comes from here:
https://dev.mysql.com/doc/refman/5.7/en/lock-tables.html
It seems like it's fine to issue `UNLOCK TABLES` even if you don't have any locks, so I think this script should always do the right thing now, regardless of ALTER TABLE unlocking or not unlocking tables.
Test Plan: Ran `bin/storage upgrade -f`, saw table end up in the right state. I'll also check this on `secure`, where the starting state is a little messier.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12987
Differential Revision: https://secure.phabricator.com/D18623
Summary:
Ref T11823. This change isn't standalone, but prepares for the more involved code change by dropping obsolete columns from the RefCursor table and adding the unique key we need to prevent the ambiguous/duplicate refs issue.
This data was moved to the RefPosition table in D18612.
Test Plan: Ran storage upgrade. See next revision for more substantial testing of this change series.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18613
Summary:
Ref T11823. This populates the new RefPosition table based on the existing RefCursor table, and deletes now-duplicate rows in the RefCursor table so the next change can add a unique key.
This change is not standalone, and there need to be separate code updates. I have a rough version of that written, but this migration needs to happen first to test it.
I'll hold this whole series of changes until after the release cut and until the code is updated.
Test Plan: Ran migration, spot-checked database tables. Saw redundant rows remove and correct-looking rows populated into the new RefPosition table.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18612
Summary:
Ref T11823. Currently, we have a "RefCursor" table which stores rows like `<branch or tag name, commit it is pointing at>` with some more data.
Because Mercurial can have a single branch pointing at several different places, this table must allow multiple rows with the same branch or tag name.
Among other things, this means there isn't a single PHID which can be used to identify a branch name in a stable way. However, we have several UIs where we want to be able to do this.
Some specific examples where we run into trouble: in Mercurial, if there are 5 heads for "default", that means there are 5 phids. And currently, if someone deletes a branch, we lose the PHID for it. Instead, we'd rather retain it so the whole world doesn't break if you accidentally delete a branch and then fix it a little later.
(I'll likely hold this until the rest of the logic is fleshed out a little more in followup changes.)
Test Plan: Ran `bin/storage upgrade`, saw the table get created without warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18602
Summary:
Fixes T12979. In D18457, we added a "No Notifications" setting to let users disable the blue and yellow pop-up notifications that alert you when an object has been updated, since some users found them distracting.
However, the change made "do nothing" the default, so all other `JX.Notification` callsites -- which never pass a preference -- were effectively turned off no matter what your setting was set to. This includes the "Read-Only" mode warning (grey), the "High Security" mode warning (purple), the "timezone" warning, and a few others.
Tweak things a little bit so the setting applies to ONLY blue and yellow ("object you're following was updated" / "this object was updated") notifications, not other types of popup notifications.
Test Plan:
- With notifications on in settings, got blue notifications and "Read-only".
- With notifications off in settings, got "Read-only" but no blue notifications.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12979
Differential Revision: https://secure.phabricator.com/D18600
Summary:
Ref T12819. This is shipping, so issue upgrade guidance to instruct installs to rebuild the index.
Also generate a new `quickstart.sql` since we haven't regenerated in a bit and there's been a large amount of table churn fairly recently.
Test Plan: Ran `bin/storage upgrade`, saw guidance notification in UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18594
Summary: Adds some side margin here.
Test Plan: error out a form field in a white box
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18571
Summary: Ref T12819. More ferret engine support.
Test Plan: Indexed and searched commits and repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18572
Summary: Ref T12819. Support for Pholio.
Test Plan: Indexed and searched mocks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18569
Summary: Ref T12819. Adds ferret engine support for Calendar events.
Test Plan: Indexed and queried calendar events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18568
Summary: Ref T12819. Adds Ferret engine support.
Test Plan: Indexed and searched for documents.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18567
Summary: Ref T12819. Adds support for projects.
Test Plan: Indexed and searched for projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18566
Summary: Ref T12819. Mostly straightforward, with a couple of minor query modernization things.
Test Plan: Indexed and searched for posts and blogs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18565
Summary: Ref T12819. Same deal as before, but smaller diffs after D18559.
Test Plan: Indexed and searched for packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18564
Summary: Ref T12819. Adds Ferret support to Passphrase.
Test Plan: Indexed credentials, searched for credentials.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18556
Summary: Ref T12819. Adds Ferret engine support to initiatives.
Test Plan: Indexed and searched for initiatives.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18555
Summary:
Ref T12819. Adds support for indexing user accounts so they appear in global fulltext results.
Also, always rank users ahead of other results.
Test Plan: Indexed users. Searched for a user, got that user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18552
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: 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: Custom icons here aren't being set. Also use more standard `tt` UI.
Test Plan: Set an icon, see set Icon.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18522
Summary: I can't seem to blame any rational for this, these should vertical align.
Test Plan: Project headers with watching buttons.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18520
Summary: This is an older CSS rule that's no longer needed on mobile.
Test Plan: view history view with harbormaster results
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18507
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
