Expose DifferentialRevisionQuery's ability to search for revisions with draft authors
Needs RevisionPublic
Actions

Authored by amckinley on Feb 12 2019, 7:08 PM.

Details

Reviewers

Summary

See https://discourse.phabricator-community.org/t/searching-for-revisions-with-unsaved-inline-comments/2378. Ref D15921. I didn't know how the commenting and draft commenting stuff worked, so I wrote this for funsies. I don't have strong feelings about actually landing this, but we should either land this, which makes this query variable reachable and working again, or a diff that actually removes the variable and the (currently broken) JOIN clause from DifferentialRevisionQuery.

Note that this query term is only accessible via Conduit, to avoid further cluttering the revision search UI.

Further context from Conpherence thread:

Roughly, I think the story is: the feature was added in D1927 in 2012 and never worked quite right. It was removed as a default filter in 2013 during a general simplification of how many filters we ship with. No one complained and it was removed completely in 2016. We can now actually build it properly, but no one has asked for it since it was removed 3 years ago and only one other person has "asked" for it in the last 7 years, in the form of D1927, which was a bootcamp task so who knows.

Test Plan

Made some revisions, commented as different users, observed expected results in the UI before setting the control to hidden.

Diff Detail

Repository

rP Phabricator

Branch

comment-search

Lint

Lint Errors

Severity	Location	Code	Message
Error	/Users/amckinley/src/phacility/core/lib/libphutil/src/filesystem/Filesystem.php:428	PHL1	Unknown Symbol
Error	/Users/amckinley/src/phacility/core/lib/libphutil/src/filesystem/Filesystem.php:568	PHL1	Unknown Symbol
Error	src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php:104	PHL1	Unknown Symbol

Unit

Tests Passed

Build Status

Buildable 21960
Build 29989: Run Core Tests
Build 29988: arc lint + arc unit

Event Timeline

amckinley created this revision.Feb 12 2019, 7:08 PM

Harbormaster completed remote builds in B21960: Diff 48116.Feb 12 2019, 7:10 PM

amckinley requested review of this revision.Feb 12 2019, 7:10 PM

Herald added a subscriber: Korvin. · View Herald TranscriptFeb 12 2019, 7:10 PM

I'd weakly sort of tend to maaaaybe favor just removing this since the use cases seem rare/weak/poorly explained (who has SOOOOO many relevant active revisions that they can't just look for the yellow bubble? I have like a few hundred on this install but I can still scan through them pretty easily?) but I don't think it hurts anything. But I don't feel strongly, and my original case for removal was more about "this is a mess that doesn't work" than "this is an obviously bad / obviously-pure-clutter feature". As far as I know, it works well now. I think it's fine to add to the UI if we keep it, even.

I'd be more convinced this is valuable if I had a clearer understanding of the use case, but the original D1927 was light on particulars and the Discourse thread isn't terribly illuminating in crafting a narrative about exactly who is using this and why they have so many drafts.

This is also somewhat conflated with the separate "users coming from GitHub don't realize inlines don't immediately submit" problem, which was historically more of a one-time user education/onboarding problem. I think we do a better job of explaining the rules now than we did in 2012, and GitHub has supported our model since ~2017ish.

If we do land this, one policy thing inline should get fixed. Other than the one inline, all the actual code looks right to me.

At the end of the day, maybe it's less work to just move forward with this than clean up all the support code enabling it. I also suspect there may truly be a compelling story behind this somewhere that explains why this is a valuable feature we should support (and it's not like one random person requested it once out of nowhere), I just don't know what that story actually is.

src/applications/differential/query/DifferentialRevisionQuery.php
217	(Could pick up an `array` typehint.)
src/applications/differential/query/DifferentialRevisionSearchEngine.php
54–56	We should throw if the list of authors is not exactly: empty; or the viewer's PHID. Otherwise, I can discover revisions which other users have open drafts on, and I write a spybot which repeatedly queries the endpoint to see if you've started drafting any inlines yet. This isn't a huge deal, but I've historically resisted features like "epriestley is currently looking at this document" since I think they're sort of creepy/micromanagey/slippery-slopey. See T3802. I'm not a "hard no" on this kind of feature, but I want us to make an explicit decision before exploring this territory rather than, say, gradually eroding the walls around it. Currently, philosophically, your drafts are your own, and are secret/private, and you're free to consider things and revise before publishing your thoughts. In theory, this attitude leads toward more considered discourse.

This revision now requires changes to proceed.Feb 12 2019, 7:34 PM

epriestley added inline comments.Feb 12 2019, 7:35 PM

src/applications/differential/query/DifferentialRevisionQuery.php
217	Oh, and `withDraftAuthorPHIDs()` / `->draftAuthorPHIDs` is slightly more modern. Very old methods just did `withAuthors(...)` or whatever but eventually that created confusion (do we pass PHIDs or user objects?), and in a handful of (non-API-facing) cases you do actually pass the objects (`withRepositories()` to CommitQuery), not just PHIDs!

Maybe another mild argument in favor of this is that the draft stuff is now a core infrastructure capability and any application can support it. Drafts don't have a very strong meaning in most other applications today (in an application like Maniphest, a "draft" just means you typed something into the comment box), but they do (or, could) in at least Audit/Pholio.

This is a pretty weak argument since it's basically "this feature isn't very clearly that good, but we can sure have a lot of it", but I do think "more efficient" features (i.e., more effect for less code) are generally better than "less efficient" features. If this is extended to Audit and Pholio in the future, there's relatively little space for obscure bugs that only affect one application in some weird way to exist.

jbrownEP added a subscriber: jbrownEP.Mar 8 2019, 11:57 PM