1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-24 06:20:56 +01:00
Commit graph

2018 commits

Author SHA1 Message Date
epriestley
156adccef0 Fix an issue where "bin/differential migrate-hunk" could decompress data
Summary:
Fixes T12986. I caught this bug in the changes from D18584: when we moved a large hunk to file storage, we would decompress it but keep the "deflated" flag. This could cause confusion when loading it later. I missed this in testing since I wasn't exhaustive enough in checking hunks and didn't run into a compressed one.

Instead of compressing on `save()`, compress during the normal workflow.

We currently never advise users to run this workflow so I didn't bother trying to clean up possible existing migrations.

Test Plan:
  - Ran `bin/differential migrate-hunk` on compressed hunks, moving them to and from file storage. Saw them work correctly and remain compressed.
  - Created new small (uncompressed) and large (compressed) hunks, verified they work properly and get compressed (if applicable).
  - Used `bin/cache purge --caches changeset` to clear changeset caches and make sure the actual table was being hit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12986

Differential Revision: https://secure.phabricator.com/D18624
2017-09-18 14:00:41 -07:00
epriestley
51b810b0eb Fix "Author's projects" Herald rules for revisions and diffs
Summary:
See PHI71. These didn't get properly updated when we wrote Subprojects and Milestones, and should use materialized members, not raw members. Swap the query so projects you are an indirect member of (e.g., milestones you are a member of the parent for, and parent projects you are a member of a subproject of) are included in the result list.

Also fix a bad typeahead datasource.

Test Plan:
  - Ran a dry run with the test console, saw project PHIDs for milestones and parent projects in the raw field value.
  - Tried to set "Author's projects" to a user, no longer could.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18619
2017-09-15 17:59:49 -07:00
epriestley
8982e3e52d Update major RefCursor callsites to work properly with RefPosition
Summary:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.

I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.

Test Plan:
  - This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
  - Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
  - Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
  - Browed around Diffusion in various repositories.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18614
2017-09-15 10:21:32 -07:00
epriestley
c310f08b7a Work around workflow blocking error with duplicate "master" refs in "Land Revision"
Summary:
Ref T11823. See PHI68. T11823 has a full description of this issue and a plan to fix it, but the full plan is relatively complicated.

Until that can happen, provide a workaround for the biggest immediate issue, where multiple copies of a ref cursor can cause `executeOne()` to throw, since it expects a single result. In practice, these copies are always identical so we can just pick the first one.

This will get cleaned up once T11823 is fixed properly.

Test Plan:
Forced the table into a duplicate/ambiguous state, reproduced a similar-looking error:

{F5180999}

Applied the patch, got the "Land" to work as expected:

{F5181000}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18599
2017-09-13 16:03:10 -07:00
epriestley
6fb3f857fb Stop the bleeding caused by attaching enormous patches to revision mail
Summary:
Ref T12033. This is a very narrow fix for this issue, but it should fix the major error: don't attach patches if they're bigger than the mail body limit (by default, 512KB).

Specifically, the logs from an install in T12033 show a 112MB patch being attached, and that's the biggest practical problem here.

I'll follow up on the tasks with more nuanced future work.

Test Plan: Enabled `differential.attach-patches`, saw a patch attached to email. Set the byte limit very low, saw patches get thrown away.

Reviewers: chad, amckinley

Reviewed By: amckinley

Maniphest Tasks: T12033

Differential Revision: https://secure.phabricator.com/D18598
2017-09-13 15:32:55 -07:00
epriestley
d15fb20fe6 Support storage of Differential hunk data in Files
Summary:
Ref T12932. For long-lived installs, one of the largest tables tends to be the hunk data table. Although it doesn't grow tremendously fast, it's also well suited to storage in Files instead of the database (infrequent access, relatively large blobs of data, mostly one-at-a-time access), and earlier work anticipated eventually adding support for Files storage.

Make Files storage work, and provide `bin/differential migrate-hunk` to manually test/migrate hunks. This is currently the only way hunks get moved to file storage, but I expect to add a GC step which moves them to File storage after 30 days shortly.

The immediate motivation for this is to relieve storage pressure on db001/db002 so we have more headroom for deploying the Ferret engine and its larger indexes (see also T12819).

Test Plan:
  - Used `bin/differential migrate-hunk` to move a hunk to and from file storage, verified it survived intact.
  - Downloaded the actual stored file, sanity-checked it. Verified permissions.
  - Destroyed a diff with `bin/remove destroy`, saw the hunk and file storage destroyed.
  - Verified that going from file -> text destroys the old file properly with `migrate-hunk --trace ...`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12932

Differential Revision: https://secure.phabricator.com/D18584
2017-09-11 16:09:02 -07:00
epriestley
2218caee0f Reduce the amount of boilerplate that implementing FerretInterface requires
Summary:
See brief discussion in D18554. All the index tables are the same for every application (and, at this point, seem unlikely to change) and we never actually pass these objects around (they're only used internally).

In some other cases (like Transactions) not every application has the same tables (for example, Differential has extra field for inline comments), and/or we pass the objects around (lots of stuff uses `$xactions` directly).

However, in this case, and in Edges, we don't interact with any representation of the database state directly in much of the code, and it doesn't change from application to application.

Just automatically define document, field, and ngram tables for anything which implements `FerretInterface`. This makes the query and index logic a tiny bit messier but lets us delete a ton of boilerplate classes.

Test Plan: Indexed objects, searched for objects. Same results as before with much less code. Ran `bin/storage upgrade`, got a clean bill of health.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12819

Differential Revision: https://secure.phabricator.com/D18559
2017-09-07 13:23:31 -07:00
epriestley
a2a2b3f7f4 Sort global fulltext results by overall relevance
Summary:
Ref T12819. Currently, under the Ferret engine, we query each application's index separately and then aggregate the results.

At the moment, results are aggregated by type first, then by actual rank. For example, all the revisions appear first, then all the tasks.

Instead, surface the internal ranking data from the underlying query and sort by it.

Test Plan: Searched for "A B" with a task named "A B" and a revision named "A". Saw task first. Broadly, saw mixed task and revision order in result sets.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12819

Differential Revision: https://secure.phabricator.com/D18551
2017-09-07 13:21:58 -07:00
epriestley
8059db894d Use the Ferret engine fulltext document table to drive auxiliary fulltext constraints
Summary:
Ref T12819. I started trying to get individual engines to drive these constraints (e.g., `ManiphestTaskQuery` can do most of the work) but this is a big pain, especially since most engines don't support "any owner" or "no owner", and not everything has an owner, and so on and so on. Going down this path would have meant a huge pile of stub functions everywhere, I think.

Instead, drive these through the main engine using the fulltext document table, which already has everything we need to apply these constraints in a uniform way.

Also tweak some parts of query construction and result ordering.

Test Plan: Searched for documents by author, owner, unowned, any owner, tags, subscribers, fulltext in global search. Got sensible results without any application-specific code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12819

Differential Revision: https://secure.phabricator.com/D18550
2017-09-07 13:21:42 -07:00
epriestley
4ea677ba97 Skeleton support for running global fulltext queries via the Ferret engine
Summary:
Ref T12819. Provides a Ferret-engine-based fulltext engine to ultimately replace the InnoDB fulltext engine.

This is still pretty basic (hard-coded and buggy) but technically sort of works.

To activate this, you must explicitly configure it, so it isn't visible to users yet.

Test Plan: Searched for objects with global fulltext search, got a mixture of matching revisions and tasks back.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12819

Differential Revision: https://secure.phabricator.com/D18548
2017-09-06 13:15:36 -07:00
epriestley
e91d72fefb Un-hide the "X added reviewers: ..." transactions in revision creation mail
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
2017-09-06 10:23:27 -07:00
Chad Little
818b90cf12 Update Create Diff page for new Edit UI
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
2017-09-06 10:14:58 -07:00
Chad Little
fc893658b8 Update menu item names for Applications -> Favorites
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
2017-09-05 19:05:03 -07:00
epriestley
f40f3ca74c Add Ferret engine index support to Differential
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
2017-09-05 16:45:37 -07:00
epriestley
b4cbea9018 Make legacy revision statuses from "differential.query" have type "string" again
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
2017-08-29 13:05:02 -07:00
epriestley
f49d103af5 Fix an issue where "Close Revision" did not appear in the UI
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
2017-08-29 09:58:48 -07:00
epriestley
213e4ec9b5 Add a missing (int) cast to diff IDs for new "transaction.search" method
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
2017-08-25 07:31:22 -07:00
epriestley
fa5bcf5d94 Provide some more detailed information about inline comments in "transaction.search"
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
2017-08-24 15:26:50 -07:00
epriestley
9639ec0dfa Slightly simplify logic for determining if an inline comment has an effect
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
2017-08-24 15:26:32 -07:00
epriestley
6c9026c33a Allow ModularTransactions to opt in to providing data to Conduit
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
2017-08-24 15:25:55 -07:00
epriestley
ba1925b155 Prevent Differential changeset HTML anchors from colliding with comment anchors
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
2017-08-24 15:25:17 -07:00
epriestley
48a74de0b6 Move all revision status transactions to modern values and mechanics
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
2017-08-12 04:05:57 -07:00
epriestley
7b695aa43b Migrate revision storage to modern status constants ("accepted") instead of legacy numeric values ("2")
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
2017-08-12 04:02:10 -07:00
epriestley
5348f34c9e Make all revision status readers explicitly read modern or legacy status
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
2017-08-11 17:22:22 -07:00
epriestley
cd15c2d545 Swap transactions and initialization over to modern status constants
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
2017-08-11 17:21:51 -07:00
epriestley
895f0cde1f Use modern revision statuses when bucketing revisions on the Differential dashboard
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
2017-08-11 17:21:27 -07:00
epriestley
7f743c14d5 Remove remaining ArcanistDifferentialRevisionStatus references in revision state logic
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
2017-08-11 17:21:09 -07:00
epriestley
2b9838b482 Modularize remaining TYPE_ACTION transactions in Differential, reducing calls to ArcanistDifferentialRevisionStatus
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
2017-08-11 17:20:55 -07:00
epriestley
19bc91fd20 Modularize the Differential "status" transaction and move away from ArcanistDifferentialRevisionStatus
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
2017-08-11 17:20:40 -07:00
epriestley
77bf245637 Continue reducing callsites to ArcanistDifferentialRevisionStatus in transactions
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
2017-08-11 13:43:21 -07:00
epriestley
36197bf783 Provide revision status information via API all "differential.revision.search"
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
2017-08-11 13:42:45 -07:00
epriestley
ef8d4e2126 Fix an inverted condition for the "Reopen Revision" action
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
2017-08-11 13:41:54 -07:00
epriestley
153e4d8a38 Remove old reviewer double writes to legacy edge table in Differential
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
2017-08-11 13:38:52 -07:00
epriestley
42020e1357 Completely remove "differential.find" Conduit API method
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
2017-08-11 13:38:28 -07:00
epriestley
13ddb15bbc Remove legacy withStatus() method from RevisionQuery
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
2017-08-11 13:37:47 -07:00
epriestley
50dfdb8d03 Replace legacy Differential queries for "open" revisions with a modern mechanism
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
2017-08-11 13:37:11 -07:00
epriestley
53516093ae Replace Differential hard-coded status "<select />" with tokenizer
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
2017-08-11 13:36:00 -07:00
epriestley
8160baec2a Add a Differential revision status tokenizer datasource
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
2017-08-11 13:35:15 -07:00
Alex Vandiver
45b0fd8f9b Remove a debugging "echo" that crept in in dccd799b
Summary: This echo was accidentally added in dccd799b

Test Plan: Inspection.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D18391
2017-08-11 05:50:43 -07:00
epriestley
7f90ef2d82 Make the "Requested Changes to Prior Diff" reviewer icon red, not bluegrey
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
2017-08-10 11:04:21 -07:00
epriestley
46d1596bf7 Pull legacy revision query status filters out of the main Query class
Summary:
Ref T2543. Currently, Differential uses a set of hard-coded query filters (like "open" and "closed") to query revisions by status (for example, "open" means any of "review, revision, changes planned, accepted [usually]").

In other applications, like Maniphest, we've replaced this with a low level list of the actual statuses, plus higher level convenience UI through tokenizer functions. This basically has all of the benefits of the hard-coded filters with none of the drawbacks, and is generally more flexible.

I'd like to do that in Differential, too, although we'll need to keep the legacy maps around for a while because they're used by `differential.find` and `differential.getrevision`. To prepare for this, pull all the legacy stuff out into a separate class. Then I'll modernize where I can, and we can get rid of this junk some day.

Test Plan: Grepped for `RevisionQuery::STATUS`. Ran queries via Differential UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18343
2017-08-09 11:06:15 -07:00
epriestley
03ab7224bb Reduce STATUS_CLOSED (now internally "Published") revision status callsites
Summary:
Ref T2543. Add `isPublished()` to mean: exactly the status 'closed', which is now interally called 'published', but still shown as 'closed' to users.

We have some callsites which are about "exactly that status", vs "any 'closed' status", e.g. including "abandoned".

This also introduces `isChangePlanned()`, which felt less awkward than `isChangesPlanned()` but more consistent than `hasChangesPlanned()` or `isStatusChangesPlanned()` or similar.

Test Plan: `grep`, loaded revisions, requested review.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18341
2017-08-09 11:05:42 -07:00
epriestley
70088f7eec Continue reducing callsites to ArcanistDifferentialRevisionStatus
Summary:
Ref T2543. Further consolidates status management into DifferentialRevisionStatus.

One change I'm making here is internally renaming "CLOSED" to "PUBLISHED". The UI will continue to say "Closed", at least for now, but this should make the code more clear because we care about "is closed, exactly" vs "is any closed status (closed, abandoned, sometimes accepted)". This distinction is more obvious as `isClosed()` vs `isPublished()` than, e.g., `isClosedWithExactlyTheClosedStatus()` or something. I think "Published" is generally more clear, too, and more consistent with modern language (e.g., "pre-publish review" replacing "pre-commit review" to make it more clear what we mean in Git/Mercurial).

I've removed the IN_PREPARATION status since this was just earlier groundwork for "Draft" and not actually used, and under the newer plan I'm trying to just abandon `ArcanistDifferentialRevisionStatus` entirely (or, at least, substantially).

Test Plan:
- Viewed revisions.
- Viewed revision list.
- Viewed revisions linked to a task in Maniphest.
- Viewed revision graph of dependencies in Differential.
- Grepped for `COLOR_STATUS_...` constants.
- Grepped for removed method `getRevisionStatusIcon()` (no callsites).
- Grepped for removed method `renderFullDescription()` (one callsite, replaced with just building a `TagView` inline).
- Grepped for removed method `isClosedStatus()` (no callsites after other changes).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18340
2017-08-09 11:05:22 -07:00
epriestley
2e36653965 Reduce callsites to "ArcanistDifferentialRevisionStatus" in Phabricator
Summary:
Ref T2543. These are currently numeric values, like "0" and "3". I want to replace them with strings, like "accepted", and move definitions from Arcanist to Phabricator.

To set the stage for this, reduce the number of callsites where Phabricator invokes `ArcanistDifferentialRevisionStatus`.

This is just the easy ones. I'll hold this until the release cut.

Test Plan:
- Called `differential.find`.
- Called `differential.getrevision`.
- Called `differential.query`.
- Removed all reviewers from a revision, saw warning.
- Abandoned the no-reviewers revision, no more warning.
- Attached a revision to a task to get it to show the state icon with the status on a tooltip.
- Viewed revision bucketing on dashboard.
- Used `bin/search index` to reindex a revision.
- Hit the "Land Revision" endpoint.

I didn't explicitly test these cases:

  - Doorkeeper Asana integration, since setup takes a thousand years.
  - Disambiguation logic when multiple hashes match, since setup is also very involved.
  - Releeph because it's Releeph.

Reviewers: chad

Reviewed By: chad

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18339
2017-08-09 11:04:52 -07:00
Chad Little
8ca29a607a Remove incorrect policy language on Diff reviewers
Summary: Fixes T12952. This never work AFAIK, so resolves this mis-information. See T4411 for follow up.

Test Plan: Click on policy for a diff, no longer see text.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12952

Differential Revision: https://secure.phabricator.com/D18349
2017-08-06 08:08:06 -07:00
epriestley
e47f85cd98 In Differential, filter repository operations to just "Land" operations again
Summary:
Reverts D18276. See PHI18 for discussion. The additional rules here (roughly, "only show the first successful operation") didn't actually work out for the other types of operations.

This is all just figuring out a stopgap, T12935 and other changes should eventually provide real pathways here.

Test Plan: Straight revert.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D18281
2017-07-26 11:51:18 -07:00
epriestley
1588d3e224 In Differential, render status for any active Drydock repository operation, not just "Land" operations
Summary:
See PHI18. Third parties can currently define other types of Drydock operations (like "Merge Check" or "Cherry-Pick") but we won't show them in the UI.

This is a simple change which improves third-party support for now. These kinds of operations generally make sense in the upstream, but the pathways to support are longer.

Test Plan:
  - Verified that there are no other types of repository operation which we'd want to exclude in the upstream today by reviewing the "Repository Operation" subclasses.
  - Will click some buttons in production to make sure this works.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D18276
2017-07-25 09:55:40 -07:00
epriestley
c217d7619c Make "A" hide or show all inline comments
Summary: Ref T12733. See PHI17.

Test Plan: Pressed "A", then pressed "A".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12733

Differential Revision: https://secure.phabricator.com/D18274
2017-07-25 05:12:39 -07:00
Chad Little
39e5da7ea7 Simplify Diffusion Browse Table
Summary: Cleans up colors, removes commit hash and links the text instead. Also unsure how valuable "lint" column is here, but left it. I'd maybe like to understand that workflow since it just seems like clutter overall. Also Fixes T12905

Test Plan:
Review Phabricator, hg, and a few other test repositories locally. Holler if anything here seems bad, but this feels easier to read and use to me.

{F5038425}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12905

Differential Revision: https://secure.phabricator.com/D18189
2017-07-09 09:43:57 -07:00
epriestley
596b83a712 Move misplaced validation for ambiguous fields in "Test Plan" to the right place
Summary:
When users use the web UI to enter text like "Reviewers: x" into the "Summary" or "Test Plan", we can end up with an ambiguous commit message.

Some time ago we added a warning about this to the "Summary" field, and //attempted// to add it to the "Test Plan" field, but it actually gets called from the wrong place.

Remove the code from the wrong place (no callers, not reachable) and put it in the right place.

This fixes an issue where users could edit a test plan from the web UI to add the text "Tests: ..." and cause ambiguities on a later "arc diff --edit".

Test Plan: {F5026603}

Reviewers: chad, amckinley

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D18175
2017-06-30 06:36:05 -07:00