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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Ref T2543. Rewrites all the storage to use constants.
Note that transactions still use legacy values, I'll migrate and update them separately.
Test Plan:
- Ran migration.
- Browsed around, changed revision states, viewed dashboard, etc.
- Selected `DISTINCT()` and `GROUP_CONCAT()` of the `status` field in the database, saw sane/expected before and after values.
- Verified that old Conduit methods still return numeric constants for compatibility.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18418
Summary: Ref T2543. Swaps these over to modern constants.
Test Plan: Viewed dashboard, no chagnes to bucketing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18414
Summary: Ref T2543. All callsites are now in terms of `withStatuses()`.
Test Plan:
- Called `differential.query` and `differential.find` from Conduit API.
- Grepped through all `withStatus()` callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18396
Summary: Ref T2543. Several queries want only open revisions. Provide a tailored, non-legacy way to issue that query.
Test Plan: Viewed some of these callsites (e.g., "Similar open revisions affecting these files"), saw only open revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18395
Summary:
Ref T2543. This updates the UI control in the web UI. Also:
- This implicitly makes this queryable with the API (`differential.revision.search`); it previously was not.
- This does NOT migrate existing saved queries. I'll do those in the next change, and hold this until it happens.
- This will break some existing `/differential/?status=XYZ` links. For example, `status=open` now needs to be `status=open()`. I couldn't find any of these in the upstream, and I suspect these are rare in the wild (users would normally link directly to saved queries, not use URI query construction).
Test Plan: {F5093611}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18393
Summary:
Ref T2543. 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
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
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
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
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
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
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
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
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
Summary: Fixes T6660. Uses the new stuff in Audit to build an EditEngine-aware icon.
Test Plan: {F2364304}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6660
Differential Revision: https://secure.phabricator.com/D17208
Summary:
Fixes T12095. Ref T6660. The old code for this was specific to Differential, using the `DifferentialDraft` table.
Instead, make the `EditEngine` / `VersionedDraft` code create and remove a `<objectPHID, authorPHID>` edge when a particular author creates drafts.
Some applications have drafts beyond `VersionedDrafts`, notably inline comments. Before writing "yes, draft" or "no, no draft", ask the object if it has any custom draft stuff we need to know about.
This should fix all the yellow bubble bugs I created in T11114 and allow us to bring the feature to Audit fairly easily.
Test Plan: Created and deleted comments and inlines, reloading the list view after each change. Couldn't find a way to break the list view anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12095, T6660
Differential Revision: https://secure.phabricator.com/D17205
Summary:
Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail.
When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too.
Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it.
Test Plan:
- Used `bin/differential attach-commit <commit> <revision>` to hit this code.
- Simulated a filesystem write failure, saw the diff get reused.
- Also did a normal update, which worked properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10968
Differential Revision: https://secure.phabricator.com/D17164
Summary: Ref T10967. This makes room for a `DifferentialReviewer` object which can be a real storage table.
Test Plan: Grepped for `DifferentialReviewer`, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17041
Summary: Ref T8475. This gets rid of most of the old "legacy hunk" code. I'll nuke the rest (and drop the old table) once we're more sure that we're in the clear.
Test Plan: Browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8475
Differential Revision: https://secure.phabricator.com/D17040
Summary:
When having lots of repos, seeing "all revisions in this project" is hard, and we ended up adding herald rules to basically copy project tags to the revisions on a per-project basis. Adding a "tagged: project" function to the Repositories search field allows users to find differentials within a project.
Fix T10850.
Test Plan: search differentials by tagging project and repository in the Repository field
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: avivey, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10850
Differential Revision: https://secure.phabricator.com/D16096
Summary:
Ref T4103. This doesn't get everything, but takes care of most of the easy stuff.
The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next.
Test Plan:
- Grepped for `loadPreferences` to identify callsites.
- Changed start-of-week setting, loaded Calendar, saw correct start.
- Visited welcome page, read "Adjust Settings" point.
- Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence.
- Enabled Filetree, toggled in Differential.
- Disabled Filetree, no longer visible in Differential.
- Changed "Unified Diffs" preference to "Small Screens" vs "Always".
- Toggled filetree in Diffusion.
- Edited a task, saw sensible projects in policy dropdown.
- Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd.
- Toggled "monospaced textareas", used a comment box, got appropriate fonts.
- Toggled durable column.
- Disabled title glyphs.
- Changed monospaced font to 18px/36px impact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16004
Summary:
Ref T10939. If you haven't installed a dashboard, we show an "Active Revisions" panel on the homepage by default. I waited a bit to update this, but the new buckets don't seem to have caused any major problems so far.
Update this to use the new logic. I'm just showing "must review" + "should review", which is similar to the old beahvior.
Also replace the notification count with this same number. This is a little different from the old behavior, but simpler, and I think we should probably move toward getting rid of these counts completely.
Test Plan:
- Viewed homepage as logged-in user, saw my revisions (including revisions I have authority over only because of project membership).
- Saw consistent notification count.
- Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15950
Summary:
Ref T10939. I don't think this is hugely important, but it doesn't clutter things up much and it's nice as a hint.
T4055 was the original request specifically asking for this. It wanted a separate bucket, but I think this use case isn't common/strong enough to justify that.
I would like to improve Differential's "X depends on Y" feature in the long term. We don't tend to use/need it much, but it could easily do a better and more automatic job of supporting review of a group of revisions.
Test Plan: {F1426636}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15930
Summary: Ref T10939. These poor stragglers got left out in the rain. Didn't catch any issues otherwise.
Test Plan: {F1426604}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15929
Summary: Ref T10939. For various historical reasons, revision status is a numeric string. This comparison fails because it's `(string) !== (int)`. Just use `!=` so this will still work if we turn it into a real string in the future.
Test Plan: Tried a more specific test case locally, got better looking results in "Must Review" and "Should Review".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15928
Summary:
Ref T10939. Fixes T9263. Ref T4144.
First, this resolves users (converting users into all packages and projects they are responsible for) earlier, so bucketing can act on that data correctly. Previously, your own blocking reviews would appear in "Must Review" but your packages/projects' would not. Now, all of them will.
Second, this adds `exact(username)` to mean "just me, not my packages/projects". You can use this along with "Bucket: By Required Action" to create a personal view of "Active Revisions" if you'd like, and ignore all your project/package reviews.
Test Plan: Queried by "me" and "exact(me)", got reasonable looking results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T9263, T10939
Differential Revision: https://secure.phabricator.com/D15925
Summary:
Ref T10939. Ref T4144. This splits the existing buckets ("Blocking Others", "Action Required", "Waiting on Others") into 6-7 buckets with a stronger focus on what the next action you need to take is.
See T10939#175423 for some discussion.
Overall, I think some of the root problems here are caused by reviewer laziness and shotgun review workflows (where a ton of people get automatically added to everything, probably unnecessarily), but these buckets haven't been updated since the introduction of blocking reviewers or project/package reviewers and I think splitting the 3 buckets into 6 buckets isn't unreasonable, even though it's kind of a lot of buckets and the root problem here is approximately "I want to ignore a bunch of stuff on my dashboard".
I didn't remove the old bucketing code yet since it's still in use on the default homepage.
This also isn't quite right until I fix the tokenizer to work properly, since it won't bucket project/package reviewers accurately.
Test Plan: {F1395972}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T10939
Differential Revision: https://secure.phabricator.com/D15924
Summary:
Ref T10939. Currently, Differential hard-codes some behaviors for the "active" filter. This introduces "buckets" to make this grouping behavior more general/flexible.
The buckets don't actually do any grouping yet, this just gets rid of the `$query === 'active'` stuff so far.
These buckets change the page size to a large value, becuase pagination won't currently work with bucketing.
The problem is that we normally paginate by selecting one more result than we need: so if we're building a page of size 10, we'll select 11 results. This is fast, and if we get 11 back, we know there's a next page with at least one result on it.
With buckets, we can't do this, since our 11 results might come back in these buckets:
- A, B, C, A, C, C, A, A, B, B, (B)
So we know there are more results, and we know that bucket B has more results, but we have no clue if bucket A and bucket C have more results or not (or if there's anything in bucket D, etc).
We might need to select a thousand more results to get the first (D) or the next (A).
So we could render something like "Some buckets have more results, click here to go to the next page", but users would normally expect to be able to see "This specific bucket, A, has more results.", and we can't do that without a lot more work.
It doesn't really matter for revisions, because almost no one has 1K of them, but this may need to be resolved eventually.
(I have some OK-ish ideas for resolving it but nothing I'm particularly happy with.)
Test Plan: {F1376542}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15923
Summary:
Ref T10939. Ref T4144. This moves the revision SearchEngine to modern code so I can add some kind of bucketing layer on top of it.
This seems to have worked pretty cleanly. One thing is that I removed the ability to search for "pending drafts":
- This was added in D1927 from a bootcamp task, was an indirect solution to a questionable problem, and almost certainly would not meet the bar today.
- Later, in D3324, we added the icons to the list. I think this is a better solution in general. In particular, it specifically addressed the query being kind of junky.
- At the time, Differential had a prebuilt "Drafts" filter. This was removed in D6347 with the move to ApplicationSearch, which simplified the large number of prebuilt filters. Although we got a lot of feedback about that, none requested that the drafts filter be restored.
Test Plan: Searched for responsible users, subscribers, orders, projects, repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T10939
Differential Revision: https://secure.phabricator.com/D15921
Summary:
Ref T10939. This has no effect yet since packages can not actually become reviewers, I'm just inching toward support.
- When searching for "responsible users", include revisions that need review by packages you have authority over.
- When calculating review authority, include authority over packages you are a member of (these currently never exist).
Test Plan:
This isn't reachable so I just `var_dump()`'d stuff and looked at the generated queries, which appeared correct/reasonable.
I'll vet this more thoroughly once packages can actually become reviewers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15909
