1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-03 04:02:43 +01:00
Commit graph

210 commits

Author SHA1 Message Date
epriestley
8e703c8c35 Provide a default "loadPage()" implementation on "CursorPagedPolicyAwareQuery"
Summary: Ref T13682. Many subclasses of "CursorPagedPolicyAwareQuery" have the same implementation of "loadPage()", and this is a sensible default behavior.

Test Plan: Examined changes to verify that all removed methods have the same behavior.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13682

Differential Revision: https://secure.phabricator.com/D21838
2022-05-24 10:18:53 -07:00
epriestley
30d58de1bc Expose "affectedPaths" to "differential.revision.search" Conduit API method
Summary: Ref T13639. Support querying by "affectedPaths" in the API.

Test Plan: {F8539347}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21621
2021-03-15 16:16:11 -07:00
epriestley
6d33ba7dc4 Move Diffusion to "withPaths()" for "Recent Open Revisions", and remove "withPath()" from DifferentialRevisionQuery
Summary: Ref T13639. Move Diffusion to use the new API and get rid of the old API now that it no longer has any callers.

Test Plan:
Grepped for remaining callers.

{F8539335}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21620
2021-03-15 16:16:11 -07:00
epriestley
925c9a71e7 Support a "withPaths()" API in DifferentialRevisionQuery, and use it on the revision view
Summary: Ref T13639. Move away from "withPath(..., ...)" to "withPaths(...)".

Test Plan: {F8539323}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21619
2021-03-15 16:16:11 -07:00
epriestley
9feb7343e6 Provide a "differential.changeset.search" Conduit API method
Summary: Ref T13605. Support selecting a diff's changesets (to get a list of affected file paths) via the API.

Test Plan: Called API with no arguments, diffPHIDs, PHIDs, IDs. Got sensible output.

Maniphest Tasks: T13605

Differential Revision: https://secure.phabricator.com/D21558
2021-02-15 11:11:13 -08:00
epriestley
2a83df5786 Fix an issue where a GROUP BY was missing when a query matched a revision using multiple hashes
Summary:
Ref T13581. If you query for revisions by hash and provide multiple hashes (A, B) which match a single revision (e.g., older and newer diffs for that revision), the query omits a GROUP BY clause but should contain one.

Add a GROUP BY clause in this case.

Test Plan:
With a working copy that has multiple hashes corresponding to a single revision, ran `arc branches` before and after the change. Before, got this error:

```
[2020-09-15 17:02:07] EXCEPTION: (ConduitClientException) ERR-CONDUIT-CORE: Rows passed to "loadAllFromArray(...)" include two or more rows with the same ID ("130"). Rows must have unique IDs. An underlying query may be missing a GROUP BY. at [<arcanist>/src/conduit/ConduitFuture.php:65]
```

After, clean execution.

Maniphest Tasks: T13581

Differential Revision: https://secure.phabricator.com/D21462
2020-09-15 17:36:42 -07:00
epriestley
5d0ae283a9 Put a readthrough cache in front of inline context construction
Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.

Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21279
2020-05-20 14:28:37 -07:00
epriestley
d2d7e7f5ff Clean up Diffusion behaviors for inline edit suggestions
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.

Test Plan:
  - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
  - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21278
2020-05-20 14:28:12 -07:00
epriestley
94a95efa05 Replace "loadUnsubmittedInlineComments()" with a modern "DiffQuery"
Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.

Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21234
2020-05-07 16:11:02 -07:00
epriestley
79107574a7 Remove "DifferentialInlineCommentQuery"
Summary: Ref T13513. Replaces "DifferentialInlineCommentQuery" with the similar but more modern "DifferentialDiffInlineCommentQuery".

Test Plan: Viewed comments in timeline, changesets. Created, edited, and submitted comments. Hid and un-hid comments, reloading (saw state preserved).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21233
2020-05-07 16:07:55 -07:00
epriestley
c1f1345cc0 Make InlineCommentQueries more robust/consistent
Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.

The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").

Test Plan: Browed, created, edited, and submitted inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21227
2020-05-07 16:00:28 -07:00
epriestley
7fa47408a3 When a user submits "isEditing" inlines and chooses to publish them, publish their current draft state as-shown
Summary: Ref T13513. When users choose to publish inlines, we want to publish the visible text, not the last "checkpointed" state.

Test Plan:
  - Created an inline ("AAA").
  - Edited it into "BBB", did not save.
  - Submitted.
  - Confirmed that I want to publish the unsaved inline.
  - Saw "BBB" publish.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21218
2020-05-04 15:16:18 -07:00
epriestley
fe501bd7f7 Save drafts for inline comments currently being edited
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.

This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.

Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21216
2020-05-04 13:19:42 -07:00
epriestley
27b7ba814a Don't consider empty inlines when considering whether a revision has draft comments or not
Summary: Ref T13513. When computing whether a revision has draft comments or not, ignore empty inlines.

Test Plan: Added empty inlines to a revision, no longer saw a yellow "draft" bubble in the list UI.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21215
2020-05-04 13:17:20 -07:00
epriestley
9307f57747 When rendering changesets, discard empty draft inline comments
Summary: Ref T13513. When you load a changeset, discard all empty inlines. This is likely a more desirable behavior than keeping empty editors around, even though the rest of the pipeline generally handles them fairly well now.

Test Plan:
  - Started an inline, didn't type any text or save, reloaded page.
    - Before: page restores empty editor in the same place.
    - After: we just discard this likely-pointless empty inline.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21214
2020-05-04 13:16:42 -07:00
epriestley
8aac55cc57 Make "Highlight As..." sticky across reloads in Diffusion and Differential
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.

The storage generates a "PhabricatorChangesetViewState" configuration object as an output.

When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.

The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.

Test Plan:
  - Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
  - Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
  - Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.

Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21137
2020-04-19 08:58:39 -07:00
epriestley
72f82abe07 Improve recovery from panel action rendering exceptions, and mark "Changeset" queries as not suitable for panel generation
Summary:
Fixes T13443. When a panel raises an exception during edit action generation, it currently escapes to top level. Instead, catch it more narrowly.

Additionally, mark "ChangesetSearchEngine" as not being a suitable search engine for use in query panels. There's no list view or search URI so it can't generate a sensible panel.

Test Plan:
  - Added a changeset panel to a dashboard.
  - Before: entire dashboard fataled.
  - After: panel fataled narrowly, menu fatals narrowly, dashboard no longer permits creation of another Changeset query panel.

Maniphest Tasks: T13443

Differential Revision: https://secure.phabricator.com/D20902
2019-11-08 17:15:21 -08:00
epriestley
353155a203 Add "modifiedStart" and "modifiedEnd" constraints to "differential.revision.search"
Summary:
Fixes T13386. See PHI1391. These constraints largely exist already, but are not yet exposed to Conduit.

Also, tweak some keys to support the underlying query.

Test Plan: Ran `differential.revision.search` queries with the new constraints.

Maniphest Tasks: T13386

Differential Revision: https://secure.phabricator.com/D20730
2019-08-22 13:34:18 -07:00
epriestley
7c1f6519e0 Support "none()" in Differential to find revisions with no (un-resigned) reviewers
Summary:
Ref T13289. In Maniphest, you can currently search for "Owner: none()" to find tasks with no owner, but there's no way to search for "Reviewers: none()" in Differential right now.

Add support for this, since it's consistent and reasonable and doesn't seem too weird or niche.

Test Plan: Searched for "Reviewers: none()", found revisions with no reviewers. Searched for "Reviewers: alice, none()", "Reviewers: alice", and "Reviewers: <no constraint>" and got sensible results.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20537
2019-05-21 12:56:59 -07:00
epriestley
ec0085fd0c Align "RevisionQuery->needCommitPHIDs()" to use Edges, not the legacy table
Summary:
Depends on D20458. Ref T13276. Although I'm not thrilled about "needCommitPHIDs()", it has a few callers, including custom fields. Allow "need + attach + get" to survive for now since they're reasonably modern, at least.

However, use edges instead of "TABLE_COMMIT" and require `need...()` + `get...()`, removing the direct `load...()`.

Also remove `RevisionQuery->withCommitPHIDs(...)`, which has no callers.

Test Plan:
- Grepped for `loadCommitPHIDs` (only two hits, the private `RevisionQuery` method).
- Called "differential.getrevision", got commits.
- Viewed a revision, saw "Commits: ...".
- Grepped for `withCommitPHIDs()`, no callers on `RevisionQuery` (some other query classes have methods with this name).
- Called "differential.query", got commits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20459
2019-04-24 05:54:38 -07:00
epriestley
d4847c3eeb Convert simple query subclasses to use internal cursors
Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.

This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.

Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20292
2019-03-19 13:00:27 -07:00
epriestley
2f10d4adeb Continue making application fixes to Phabricator for changes to %Q semantics
Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.

Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19790
2018-11-15 03:50:02 -08:00
epriestley
98690ee326 Update many Phabricator queries for new %Q query semantics
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.

Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: hach-que

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19789
2018-11-15 03:48:10 -08:00
epriestley
4f54d483d5 Support export of revisions to Excel/CSV/JSON/etc
Summary: Ref T13210. See PHI806. This enables basic export of revisions into flat data formats. This isn't too fancy, but just covers the basics since the driving use case isn't especially concerned about getting all the fields and details.

Test Plan: Exported some revisions into JSON, got sensible output.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13210

Differential Revision: https://secure.phabricator.com/D19743
2018-10-11 13:34:33 -07:00
epriestley
c6c1893dc0 Allow revisions to be filtered by created date
Summary:
Ref T13202. See PHI906. This is a reasonable capability which we support in some other applications already.

(The only real reason not to support this is that it creates some clutter in the UI, but I think we're generally in better shape now than we were in the past, and we could make this UI collapse/fold at some point.)

Test Plan: Ran queries with a minimum date, a maximum date, both, and neither. Saw appropriate results in all cases.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13202

Differential Revision: https://secure.phabricator.com/D19732
2018-10-05 12:28:20 -07:00
epriestley
8c8e7f07b5 Add a standalone view for browsing changesets of very large revisions
Summary: Ref T13110. Installs have various reasons for sending unreviewable changes (changes where the text of the change will never be reviewed by a human) through Differential anyway. Prepare for accommodating this more gracefully by building a standalone changeset list page which paginates the changesets.

Test Plan: Clicked the new "Changeset List" button on a revision, was taken to a separate page.

Maniphest Tasks: T13110

Differential Revision: https://secure.phabricator.com/D19295
2018-04-05 06:35:06 -07:00
epriestley
e40aec0210 When a revision has more than 7 reviewers, render only the first 7 in the list view
Summary:
See PHI489. Ref T13110. At least for now, this just shows "..." at the end since you can click the revision to see the whole list anyway.

Also remove the older-style external Handle passing in favor of lazy construction via HandlePool.

Test Plan: Viewed revisions, fiddled with the 7 limit, got sensible-seeming "..." behavior.

Maniphest Tasks: T13110

Differential Revision: https://secure.phabricator.com/D19293
2018-04-03 12:47:43 -07:00
epriestley
f43d08c2bb Completely remove the legacy hunk table
Summary: Depends on D19056. Fixes T8475. Ref T13054. Merges "ModernHunk" back into "Hunk".

Test Plan: Grepped for `modernhunk`. Reviewed revisions. Created a new revision. Used `bin/differential migrate-hunk` to migrate hunks between storage formats and back.

Maniphest Tasks: T13054, T8475

Differential Revision: https://secure.phabricator.com/D19057
2018-02-10 16:12:50 -08:00
epriestley
12e6106a59 Refine bucketing and display rules for voided "Accepts" in Differential
Summary:
See PHI190. This clarifies the ruleset a bit:

  - If you accepted, then the author used "Request Review" explicitly, we now show "Accepted Earlier" instead of "Accepted" in the "Reviewers" list on the main revision page. This makes it sligthly more clear why the revision is back in your review queue without picking through the transaction log.
  - Instead of moving all non-current accepts into "Ready to Review", move only voided accepts into "Ready to Review". This stops us from pulling older accepts which haven't been voided (which could have been incorrectly pulled) and correctly pulls older, voided accepts from before an update (for example: accept, then request review, then update) and generally aligns better with intent/expectation.

Test Plan:
  - Accepted, requested review.
  - Saw reviewer as "Accepted Earlier".
  - Saw review in "Ready to Review" bucket.
  - Accepted, updated (with sticky accept).
  - Saw reviewer as "Accepted Prior Diff".
  - Saw review as "Waiting on Authors".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18764
2017-11-07 15:35:11 -08:00
epriestley
6ecdadb76a After "Request Review", move revisions with voided "Accepts" into "Ready to Review", not "Waiting on Other Reviewers"
Summary:
Depends on D18756. Fixes T12539. See PHI190. Currently, when this occurs:

  - Alice accepts.
  - Bailey requests review.
  - Alice views her dashboard.

...the revision appears in "Waiting on Other Reviewers" (regardless of whether other reviewers actually exist or not).

Instead, ignore these voided/non-current accepts and let the revisions appear in "Ready to Review", which is more natural.

Test Plan: Went through the steps above. On `master`, saw revision in "Waiting on Other Reviewers". After patch, saw it in "Ready to Review".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12539

Differential Revision: https://secure.phabricator.com/D18757
2017-11-01 17:19:54 -07:00
epriestley
0da3f34728 Provide "differential.diff.search"
Summary: See PHI90. For now, this only provides a limited amount of information, but should satisfy the use case in PHI90 and build toward a more complete version in the future.

Test Plan: Used new Conduit method to retrieve information about diffs.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18744
2017-10-30 15:06:10 -07:00
epriestley
683df399e7 Simplify UNION/ORDER query construction in DifferentialRevisionQuery
Summary: Ref T12680. Use the slightly sleeker construction from D18722 in Differential.

Test Plan: Viewed revision list, reordered by date modified.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12680

Differential Revision: https://secure.phabricator.com/D18727
2017-10-23 16:17:47 -07:00
epriestley
23867c1487 Add a "Draft" state for revisions, and action bucket support
Summary:
Ref T2543. There's no way to put revisions into this state yet, but start adding support for when there is.

Adds the status constant, plus support for bucketing them.

Test Plan:
  - Manually put a revision in "Draft" state by updating the database directly.
  - Verified my drafts showed up in a "Drafts" section on the bucket view.
  - Verified others' drafts did not appear on the action bucket view.
  - Viewed revisions, queried for "Draft" revisions, etc (stuff we get for free).

{F5186781}

{F5186782}

{F5186783}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18625
2017-09-18 14:01:00 -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
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
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
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
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
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
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
ffab52f17e Restrict Differential buckets to just ApplicationSearch views
Summary: Ref T9363, If we're in a dashboard panel, only show buckets with data, or a fallback if nothing exists.

Test Plan: Test 'active revisions' panel in a dashboard and in Differential.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9363

Differential Revision: https://secure.phabricator.com/D17544
2017-03-23 12:09:44 -07:00
epriestley
8913552970 Store "resigned" as an explicit reviewer state
Summary:
Fixes T11050. Today, when a user resigns, we just delete the record of them ever being a reviewer.

However, this means you have no way to say "I don't care about this and don't want to see it on my dashboard" if you are a member of any project or package reviewers.

Instead, store "resigned" as a distinct state from "not a reviewer", and treat it a little differently in the UI:

  - On the bucketing screen, discard revisions any responsible user has resigned from.
  - On the main `/Dxxx` page, show these users as resigned explicitly (we could just hide them, too, but I think this is good to start with).
  - In the query, don't treat a "resigned" state as a real "reviewer" (this change happened earlier, in D17517).
  - When resigning, write a "resigned" state instead of deleting the row.
  - When editing a list of reviewers, I'm still treating this reviewer as a reviewer and not special casing it. I think that's sufficiently clear but we could tailor this behavior later.

Test Plan:
  - Resigned from a revision.
  - Saw "Resigned" in reviewers list.
  - Saw revision disappear from my dashboard.
  - Edited revision, saw user still appear as an editable reviewer. Saved revision, saw no weird side effects.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11050

Differential Revision: https://secure.phabricator.com/D17531
2017-03-22 09:50:50 -07:00
epriestley
0ceab7d36f Rename "getReviewerStatus()" to "getReviewers()"
Summary:
Ref T10967. Improves some method names:

  - `Revision->getReviewerStatus()` -> `Revision->getReviewers()`
  - `Revision->attachReviewerStatus()` -> `Revision->attachReviewers()`
  - `Reviewer->getStatus()` -> `Reviewer->getReviewerStatus()` (this is mostly to make this more greppable)

Test Plan:
  - bunch o' `grep`
  - Browsed around.
  - If I missed anything, it should fatal in an obvious way. We have a lot of other `getStatus()` calls and it's hard to be sure I got them all.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17522
2017-03-20 17:11:40 -07:00
epriestley
a15df4f8d5 Rename "needReviewerStatus()" into "needReviewers()"
Summary: Ref T10967. The old name was because we had a `getReviewers()` tied to `needRelationships()`, rename this method to use a simpler and more clear name.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17519
2017-03-20 16:46:16 -07:00
epriestley
d179d0150c Remove obsolete "relationships" code from Differential
Summary:
Ref T10967. There have been two different ways to load reviewers for a while: `needReviewerStatus()` and `needRelationships()`.

The `needRelationships()` stuff was a false start along time ago that didn't really go anywhere. I believe the idea was that we might want to load several different types of edges (subscribers, reviewers, etc) on lots of different types of objects. However, all that stuff pretty much ended up modularizing so that main `Query` classes did not need to know about it, so `needRelationships()` never got generalized or went anywhere.

A handful of things still use it, but get rid of them: they should either `needReviewerStatus()` to get reviewer info, or the ~3 callsites that care about subscribers can just load them directly.

Test Plan:
  - Grepped for removed methods (`needRelationships()`, `getReviewers()`, `getCCPHIDs()`, etc).
  - Browsed Diffusion, Differential.
  - Called `differential.query`.

It's possible I missed some stuff, but it should mostly show up as super obvious fatals ("call needReviewerStatus() before getReviewerStatus()!").

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17518
2017-03-20 16:45:48 -07:00
epriestley
dccd799b1b Move many "reviewers" readers to new storage
Summary:
Ref T10967.

When we query for revisions with particular reviewers, use the new table to drive the query.

When we load revisions for use in the application, also use the new table to drive the query.

This doesn't convert everything: there's some old `loadRelationships()` stuff still using the old table. But this moves the major stuff over.

(This also changes the icon for "commented" from a question mark to a speech bubble.)

Test Plan:
  - Viewed revision lists and detail views on old and new code, saw identical outcomes.
  - Updated revisions, accepted/rejected/commented on revisions.
  - Hit the "Accepted Older" and "Commented Older" states by taking an action and then updating.
  - Grepped for removed methods (like `getEdgeData()` and `getDiffID()`).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17517
2017-03-20 16:45:28 -07:00
epriestley
6c21646b5f Put revisions waiting on other reviewers in their own bucket
Summary: Fixes T12323. See that task for discussion.

Test Plan: {F3424441}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12323

Differential Revision: https://secure.phabricator.com/D17425
2017-02-27 10:47:15 -08:00
epriestley
c5fa7421c2 Allow commits to be queried by repository using the tagged(...) typehaead function
Summary:
Fixes T12322. Allows you to search for commits using the `tagged(...)` repository function, so you can find "any commmit in any repository tagged with android" or similar.

I moved the function from Differential (which was the application using it) to Diffusion (which is more accurately the application which provides it).

I fixed a bug where searching for `tagged(xyz)` would have no effect (constraint was ignored) if there were no repositories tagged with "xyz". The fix isn't perfectly clean, but should work properly for the moment.

Test Plan:
  - Searched with `tagged(...)` in Diffusion and Differential.
  - Searched by repository.
  - Searched with `tagged(...)` for a project with no tagged repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12322

Differential Revision: https://secure.phabricator.com/D17426
2017-02-27 10:46:55 -08:00