Summary: Fixes T3528. We won't be able to load the user if they've been deleted, and will fatal a few lines later on `$user->getID()`.
Test Plan: I'm going with my gut on this one.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3528
Differential Revision: https://secure.phabricator.com/D6442
Summary: rPad17c99c1b0222292a47ca79561a356cb8b5a5d5 stopped the fatal and this provides the forward fix. I think this is what a forward fix is anyway.
Test Plan: viewed a revision (D63 is my boy) and no fatals
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6448
Summary:
Ref T2852. Asana has one bug which I'm having a little trouble figuring out. I want to get more information to debug it, but I'll need them to run `bin/feed republish <story_id>` to get that data.
Right now, it's incredibly hard to figure out the story ID for feed stories. So mostly this is to make that easier (click permalink; pull it out of the URL), but it also adds a little functionality and cleans the code up a bit.
The page itself could be prettier and maybe some day we'll add comments or whatever, but it seems reasonably functionalish.
Test Plan:
{F49962}
- Also loaded many pages of feed history to check that nothing broke.
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6440
Summary:
Ref T2852.
- Respect the existing setting for `"[Differential]"`.
- Show `[Request, X lines]` to make this more similar to the email.
Test Plan:
Sync'd to asana and got a task with the right subject:
{F49950}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6439
Summary:
Ref T2852. It's a little tricky to figure out Asana workspace IDs right now. If the viewer has a linked account, just pull their workspaces and show them which IDs are available.
(In theory, we could use a `<select>`, but it would have more edge cases; this seems like a pretty solid fix.)
Test Plan: {F49938}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6437
Summary:
Fixes T2691. Now, all PhabricatorActionListViews in the codebase setObjectHref to $request->getRequestURI. This value is passed over to PhabricatorActionItems right before they are rendered. If a PhabricatorActionItem is a workflow and there is no user OR the user is logged out, we used this objectURI to construct a log in URI.
Potentially added some undesirable behavior to aggressively setUser (and later setObjectURI) from within the List on Actions... This should be okay-ish unless there was a vision of actions having different user objects associated with them. I think this is a safe assumption.
Test Plan: played around with a mock all logged out (Ref T2652) and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2691
Differential Revision: https://secure.phabricator.com/D6416
Summary: Fixes T3525. This feels way better, although it's still a little hard for me to pick out of lists with otherwise default-colored items.
Test Plan: {F49910} {F49911}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3525
Differential Revision: https://secure.phabricator.com/D6435
Summary:
- The dialog has no body text.
- It uses an "Ignore" link because it predates action items.
Test Plan:
{F49894}
{F49895}
{F49896}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6433
Summary: Currently, we always show "current picture" even if you don't have one. In this case, the first available picture becomes the "current picture". Instead, show "current picture" only if there's actually a current picture to show.
Test Plan: {F49875}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6432
Summary: Fixes T3517. Moves the email verification page out of People and into Auth. Makes it look less awful.
Test Plan: {F49636} {F49637}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3517
Differential Revision: https://secure.phabricator.com/D6425
Summary: One place used status, other used state. Killed state in favor of status.
Test Plan: None at all
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6422
Summary:
Ref T3306. This interface has a hard time balancing security/policy issues and I'm not sure what the best way forward is. Some possibilities:
# We just let you see everything from the web UI.
- This makes debugging easier.
- Anyone who can see this stuff can trivially take over any user's account with five seconds of work and no technical expertise (reset their password from the web UI, then go read the email and click the link).
# We let you see everything, but only for messages you were a recipient of or author of.
- This makes it much more difficult to debug issues with mailing lists.
- But maybe we could just say mailing list recipients are "public", or define some other ruleset.
- Generally this gets privacy and ease of use right.
# We could move the whole thing to the CLI.
- Makes the UI/UX way worse.
# We could strike an awkward balance between concerns, as we do now.
- We expose //who// sent and received messages, but not the content of the messages. This doesn't feel great.
I'm inclined to probably go with (2) and figure something out for mailing lists?
Anyway, irrespective of that this should generally make things more clear, and improves the code a lot if nothing else.
Test Plan:
{F49546}
- Looked at a bunch of mail.
- Sent mail from different apps.
- Checked that recipients seem correct.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3306
Differential Revision: https://secure.phabricator.com/D6413
Summary: Ref T3306. I'm going to add more information about To/Cc here, but here's a little cleanup first.
Test Plan: {F49524}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3306
Differential Revision: https://secure.phabricator.com/D6410
Summary:
Ref T3306. Moves this from the web to the CLI, which is a tiny bit clunkier but way better as far as policies go and more repeatable for development.
See discussion in D6413.
Test Plan: Ran `bin/mail receive-test`, verified mail was received. Used and abused various options.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3306
Differential Revision: https://secure.phabricator.com/D6417
Summary:
Keep track of the state of a reviewer in an edge between reviewer and revision.
The edge stores the state of the review, added or rejected. And if the revision was
accepted by that reviewer the id of the diff accepted.
Test Plan:
Create diffs and clowncopterize reviewer list changes. This includes:
* Adding new reviewers
* Resigning
* Commandering a revision
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6372
Conflicts:
src/applications/differential/editor/DifferentialCommentEditor.php
Summary:
These versions are broken, but package distros seem to be picking them up. :/
Since the error you get is completely useless, fatal immediately with a useful message.
Ref T2594.
Test Plan: Faked verisions and hit the issue.
Reviewers: btrahan
Reviewed By: btrahan
CC: brennantaylor, Arijit, aran
Maniphest Tasks: T2594
Differential Revision: https://secure.phabricator.com/D6415
Summary: See discussion in D6403.
Test Plan: {F49488}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6409
Summary: Fixes T3481. Sort of - this thing be very ugly. Also it assumes that you'll "always" want to sign terms. I was thinking in a future diff that should be optional as well as configurable, though it was unclear to me if either was worth pursuing... Generally very hideous as the three main elements (PHUIDocument, AphrontErrorView, and AphrontForm with an AphrontFormInset) have never really played together before.
Test Plan: agreed to some test terms. noted UI displayed nicely. reloaded and noted UI told me I had signed it already. Went to different terms and filled them out wrong and got sensical errors.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3481
Differential Revision: https://secure.phabricator.com/D6399
Summary:
Audit has some weird rules about who gets email. Make them less weird:
- When a user does "Add Auditors", email the auditors.
- When a commit is commented on, email anyone in "concern" or "audit required" states.
@staticshock, my claim that I can't reproduce was crazy, I just have weird test data and read it wrong.
Test Plan: Added a user to an audit and verified they got an email.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: staticshock, aran
Differential Revision: https://secure.phabricator.com/D6414
Summary:
1. Show add reviewer typehead when user selects resign as a reviewer.
2. Change the label for add reviewers typehead when user selects resign as a reviewer.
Test Plan:
1. Add yourself as a reviewer in a diff.
2. Select "Resign as Reviewer" in comment editor.
Add reviewer typehead should display, with label "Suggest Another Reviewer".
Add reviewer typehead is also displayed after user refreshed the page with "Resign as Reviewer"
selected.
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: aran, epriestley, akramer, person
Differential Revision: https://secure.phabricator.com/D6340
Summary:
This leaves the space between the properties and the blurb looking a bit empty, but there will be more stuff there soon (status, VCS names, email, phone/fax numbers, etc., and custom user fields).
I removed "view lint messages" since I'm pretty sure no one has ever clicked it. I think providing better search (e.g, T2625) to that UI in Diffusion is a preferable approach.
Test Plan: {F49423}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6403
Summary: I'll probably move "away" to a custom field, but this is technically broken right now.
Test Plan: {F49416}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6402
Summary:
Ref T1703. Drive "user since" with a custom field and make the other fields render into a property list.
Users can make their profiles a little more personal/obnoxious now.
Also delete a bunch of code.
Test Plan: {F49415}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6401
Summary: There are only a total of three options now, merge them into a single panel.
Test Plan:
Set all the settings.
{F49406}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6400
Summary:
Ref T1703. Move profile pictures to a separate, dedicated interface. Instead of the 35 controls we currently provide, just show all the possible images we can find and then let the user upload an additional one if they want.
Possible improvements to this interface:
- Write an edge so we can show old profile pictures too.
- The cropping/scaling got a bit buggy at some point, fix that.
- Refresh OAuth sources which we're capable of refreshing before showing images (more work than I really want to deal with).
- We could show little inset icons for the image source ("f" for Facebook, etc.) instead of just the tooltips.
Test Plan:
Chose images, uploaded new images, hit various error cases.
{F49344}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2919, T1703
Differential Revision: https://secure.phabricator.com/D6398
Summary: Ref T1703. Put this stuff on Profile -> Edit Profile instead of spread across Settings -> Account and Settings -> Profile. Makes it custom-field driven.
Test Plan: {F49333} {F49334}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6397
Summary:
Depends on D6395.
- Now that inline rules have explicit priorities, they can just go in applications in all cases.
- We don't need the inline rule conditionals anymore after D6395.
Test Plan: Wrote remarkup with mentions, phriction links, countdowns, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6396
Summary:
We have this old view which is only used in two places and looks the same but has totally different markup. Get rid of it.
@chad, I'm generally going to move the user/project profiles a step toward looking like other object detail view with the custom field stuff. Not sure if you have any grand vision here; we can easily do something else later since this is like 80% "delete weird epriestley one-offs that don't look quite right in favor of standard elements".
Test Plan: {F49324} {F49325} {F49326}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6394
Summary: The once-choppy LDAP waters seem to have calmed down a bit. Use the service profile log to get a pretty good idea of what's going on with LDAP (see D6391) instead of invasive logging to get a slightly better idea.
Test Plan:
$ ~/src/php-src/sapi/cli/php -f ./bin/auth ldap --trace
>>> [2] <connect> phabricator2_auth
<<< [2] <connect> 1,755 us
>>> [3] <query> SELECT * FROM `auth_providerconfig` ORDER BY id DESC
<<< [3] <query> 423 us
Enter LDAP Credentials
LDAP Username: ldapuser
>>> [4] <exec> $ stty -echo
<<< [4] <exec> 10,370 us
LDAP Password: >>> [5] <exec> $ stty echo
<<< [5] <exec> 6,844 us
Connecting to LDAP...
>>> [6] <ldap> connect (127.0.0.1:389)
<<< [6] <ldap> 12,932 us
>>> [7] <ldap> bind (sn=ldapuser,ou=People, dc=aphront, dc=com)
<<< [7] <ldap> 6,860 us
>>> [8] <ldap> search (ou=People, dc=aphront, dc=com, sn=ldapuser)
<<< [8] <ldap> 5,907 us
Found LDAP Account: ldapuser
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6392
Summary:
First, I broke the addIcon() call recently with bad greps.
Second, a user is reporting an issue on GitHub (https://github.com/facebook/phabricator/issues/346) which I can't reproduce but which could reasonably occur for various reasons. Don't depend on being able to find the source/target of a move.
Test Plan: Looked at recent updates in Phriction.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, seungrye
Differential Revision: https://secure.phabricator.com/D6389
Summary: We currently don't read/save this value correctly. Fix the issue. Ref T1536.
Test Plan: Set real name attributes to "x, y".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, colegleason
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6388
Summary: Ref T2852. When a Differential revision is linked to an Asana task, show the related task in Differential.
Test Plan: {F49234}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6387
Summary: Ref T1703. Put a more reasonable UI than "blob of JSON" on top of this.
Test Plan:
Reordered, enabled and disabled user profile fields.
{F49317}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6393
Summary: this let's the list controller save a query. Fixes T3488. Note didn't bother denormalizing document body at all since I don't think we want to show a snippet.
Test Plan: viewed a list of legalpad documents - yay. viewed a legalpad document - yay. created a legalpad document - yay. edited a legalpad document - yay. edited with N authors - yay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3488
Differential Revision: https://secure.phabricator.com/D6382
Summary: ref T2691. These actions should be visually disabled if user not logged in consistently. Tokens was the odd one out, staying active regardless of user status.
Test Plan: viewed a mock logged out and verified "give token" was inactive from a UI-sense
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2691
Differential Revision: https://secure.phabricator.com/D6385
Summary:
Ref T2852. Asana sync tasks currently have a standard retry/backoff schedule, but the defaults are quite aggressive (retry every 60s forever). Instead, retry at increasing intervals and stop retrying after a few tries.
- Retry at intervals and stop retrying after a few iterations.
- Modernize some interfaces.
- Add better information about retry behaviors to the web UI.
Test Plan: {F49194}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6381
Summary: Fixes T3479
Test Plan: queried for contributors and got good results. tried a complex query with all possible values specified and got results when appropos.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3479
Differential Revision: https://secure.phabricator.com/D6384
Summary: Fixes T3501. `apc.stat` should generally be 0 in production and 1 in development. Raise a setup warning if it isn't.
Test Plan:
Hit both setup warnings.
{F49176}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3501
Differential Revision: https://secure.phabricator.com/D6376
Summary: Trying to show tasks & diffs in the typeahead. My brain has got dumber as I have not been in touch with phab dev. Waiting for your comments and pointers.
Test Plan:
{F47352}
The tasks should show up in the type-ahead.
Reviewers: epriestley, Afaque_Hussain
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan, blc
Maniphest Tasks: T2948
Differential Revision: https://secure.phabricator.com/D6175
Summary: Fixes T3503. "HTTPFuture" is specialized and HTTP-only; "HTTPSFuture" is generalized and HTTP+HTTPS.
Test Plan: yeah not so much
Reviewers: btrahan, edward
Reviewed By: edward
CC: aran
Maniphest Tasks: T3503
Differential Revision: https://secure.phabricator.com/D6378
Summary: Fixes T3473, mostly reverts previous changes to clean up required field text, will have to redesign that in general for responsiveness.
Test Plan: use logout form, use new conpherence form
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3473
Differential Revision: https://secure.phabricator.com/D6371
Summary:
Supports !unsubscribe and commenting on replies. Subscribers get mailed something reasonable. Fixes T3480.
Sneaks in /LX/ support. In the near future I want to have that /LX/ be a clean "signature" page sans all the edit actions and other fluff... Will resolve this as part of T3481.
Test Plan: used the metamta console to add comments and unsubscribe. added a phlog() inside mail code to verify mail bodies looked okay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3480
Differential Revision: https://secure.phabricator.com/D6369
Summary: Ref T603. Show object visibility in the UI. This isn't editable or mutable yet, but will be after T2222.
Test Plan: {F48689} {F48690}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6361
Summary:
got some basics here --
- can create document
- creates document object and document body object and cross-reference
- can update document
- creates document body object and updates reference from document object
- contributors stored correctly
- a contributor is anyone who has created or updated a legal document
- can subscribe to documents
- can flag documents
- can comment on documents
- can query for documents based on creator and create range
- uses basically modern stuff
Missing stuff --
- T3488
- T3483
- T3482
- T3481
- T3480
- T3479
Test Plan: TRUNCATED the database. From scratch made 3 legal docs. Verified versions and version were correct in document and document body database entries respectively. Left comments and verified versions and version did not update. Left updates and verified those updated versions and version. Flagged document and verified it showed up on homepage. Subscribed and verified transaction showed up.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D6351
Summary: Minor tweaks for consistency, and raise a friendlier error if the user doesn't upload anything.
Test Plan:
{F48686}
{F48685}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6360
Summary: Fixes spacing in Differential revision detail and Diffusion browse views.
Test Plan:
{F48677}
{F48678}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6359
Summary: This reverts commit e70bb28ea0. We didn't end up using these.
Test Plan: Looked at Differential.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6357
Summary: Ref T3485. Moves flag icon inline in the header.
Test Plan: {F48654}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3485
Differential Revision: https://secure.phabricator.com/D6355
Summary:
Ref T603. Ref T2625. Fixes T3241. Depends on D5451. Depends on D6346.
@wez, this changes the Differential revision list UI substantially and may generate a lot of bikeshedding / who-moved-my-cheese churn. See T3417 for context, for example. The motivations for this change are:
- The list now works on devices, like phones and tablets. This is a requirement to make the rest of Differential work on devices.
- Although ApplicationSearch intentionally presents a simpler interface initially and some options which were one click away before aren't now, it is much more powerful than the search it replaces and allows users to build, save, share, fork, edit, and customize a much wider range of queries. Users who used the old filters frequently can use Advanced Search -> Save Custom Query to create new versions of them, and of any other query. "Edit Queries.." allows users to remove and reorder queries, including builtin queries. Basically, there are like three things which have gone from "1-click" to "a few clicks", and ten trillion things which have gone from "hard/impossible" to "relatively easy".
The local screenshots look a bit iffy, but I think a lot of this is the fakenesss of my test data. If they still feel iffy in production we can tweak them until they feel good, like we did for Maniphest.
Test Plan:
{F48477}
{F48478}
Reviewers: btrahan, chad, wez
Reviewed By: btrahan
CC: aran, s
Maniphest Tasks: T603, T2625, T3241
Differential Revision: https://secure.phabricator.com/D6347
Summary:
Tried out `PhabricatorObjectItemView` for Differential. It looks smexy and smooth.
Refs T2014
- Title and Date as Maniphest
- Author in the handle icon
- Bar color reflects revision status (Needs Review, Accepted, Abandoned etc.) @chad looking for non-blue is faster than keeping watch for everything that's not "Closed" in old table form
- Some status information are in footer icons; currently only stale/old status display as well as saved drafts, maybe more in future; these come into my mind:
- No reviewer warning
- Push Blocking Priority (T2730)
- Trivial, fast review guaranteed
- Sketch / Just looking for advice/help
- Arcanist Project (T2614)
- Denote "Public Send-in" (T1476)
{F37662}
{F37663}
{F37664}
{F37665}
Some flaws:
- Date and reviewers on every entry the same?
- No respect for Differential fields (for some reason, every entry appeared the same, so broke it to parts)
- Plenty of (potential) increase in height - advise reducing paging length from 100 to 50 - or just ignore me
Suggestions for the future:
- Expand the meta information regarding revisions; e.g. the various status displays above
- Uh... T2543, T1279, T793, T731 and what else I want for Differential, because they are awesome!
- T793 should be in particular easy appearance-wise, just copy-paste from Maniphest
Test Plan: By looking at it, of course. Verified there are no errors or crashed
Reviewers: epriestley, chad, btrahan, liguobig
Reviewed By: chad
CC: aran, Korvin, edward, nh
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D5451
Conflicts:
src/__celerity_resource_map__.php
Summary: This allows the SavedQuery to modify what the result list looks like (e.g., include display flags and similar).
Test Plan: Looked at some ApplicationSearch apps.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6346
Summary:
Ref T603. Ref T2625. Use cursors to page Differential queries, not offsets.
The trick here is that some queries are ordered. In these cases, we either need to pass some kind of tuple or do a cursor lookup. For example, if you are viewing revisions ordered by `dateModified`, we can either have the next page be something like:
?afterDateModified=2398329373&afterID=292&order=modified
...or some magical token:
?afterToken=2398329373:292&order=modified
I think we did this in Conpherence, but one factor there was that paging orders update with some frequency. In most cases, I think it's reasonable to pass just the ID and do a lookup to get the actual clause value (e.g., go look up object ID 292 and see what its dateModified is) and I think this is much simpler in general.
Test Plan: Set page size in Differential to 3, and paged through result lists ordered by date created and date modified.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6345
Summary:
Ref T603. Ref T2625. Makes `DifferentialRevisionQuery` do policy checks.
Note that it still uses inefficient offset-based paging, but it's rare to page through revisions. I'll switch to cursor paging in a future diff.
Test Plan: Viewed a bunch of Differential interfaces, home page, etc. This shouldn't actually materially impact anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6344
Summary:
Currently, when querying for responsible users (revisions where a given user is either the author or a reviewer), we do this:
- If the query passes a bunch of hard-coded special cases, use a special hard-coded UNION.
- Otherwise, use a very complicated JOIN/WHERE clause.
This is bad for several reasons:
- Tons and tons of hard-coding and special casing.
- The JOIN/WHERE clause performs very poorly for large datasets.
- (As a material consequence, the homepage issues a responsible query which barely misses the hard-coded special cases and goes down the slow path.)
Instead, //always// use the UNION strategy to execute a "responsible" query. Specifically, if we have responsible PHIDs, temporarily add them to the author list and build a normal query, then repeat for reviewers, then UNION any clauses we built.
Fixes T3377. Ref T603. Ref T2625. Depends on D6342.
There's various folklore about UNION ALL / UNION DISTINCT performance. UNION DISTINCT is simpler here and the number of rows is small, although we could use UNION ALL in the form:
SELECT * FROM ((SELECT ...) UNION ALL (SELECT ...) ORDER) GROUP LIMIT
...if we find that there's some performance benefit at some point.
Test Plan: Used DarkConsole to examine queries. Viewed home page and Differential dashboard/authors/subscribers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625, T3377
Differential Revision: https://secure.phabricator.com/D6343
I don't actually have Phabricator installed locally so I have no idea if this works, but in theory this should add one of the + buttons in the left sidebar to quickly add a new macro.
Summary: They seem to look OK?
Test Plan: {F48529} {F48530}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6350
Summary:
Ref T603. This introduces a policy-aware DifferentialDiffQuery and converts most callsites.
I've left unusual callsites (mostly: hard to get the viewer, unusual query, queries related to active diffs) alone for now, so this isn't exhaustive but hits 60-80% of sites.
Test Plan: Created diff; created revision; viewed diffs and revisions; made additional conduit calls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6338
Summary: Ref T603. This query isn't policy-aware yet, but prepare for it to be one day.
Test Plan: Looked at: home page; differential home; differential detail; diffusion browse. Made differential.query conduit call.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6337
Summary: Ref T603. This is a very old, very bad version of `DifferentialRevisionQuery`. I want to modernize only the latter. Express the remaining callsite of the former in terms of `DifferentialRevisionQuery`.
Test Plan: Executed all four modes of `differential.find`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6335
Summary:
Ref T603.
- Primarily, this gets rid of a `DifferentialRevisionListData` callsite.
- Also modernize and clean up some UI stuff.
Test Plan:
{F48260}
{F48261}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6334
Summary:
- Add GC support to conduit logs.
- Add Query support to conduit logs.
- Record the actual user PHID.
- Show client name.
- Support querying by specific method, so I can link to this from a setup issue.
@wez, this migration may not be fast. It took about 8 seconds for me to migrate 800,000 rows in the `conduit_methodcalllog` table. This adds a GC which should keep the table at a more manageable size in the future.
You can safely delete all data older than 30 days from this table, although you should do it by `id` instead of `dateCreated` since there's no key on `dateCreated` until this patch.
Test Plan:
- Ran GC.
- Looked at log UI.
- Ran Conduit methods.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Differential Revision: https://secure.phabricator.com/D6332
Summary:
Ref T603. Ref T2625.
Long chain of "doing the right thing" here: I want to clean this up, so I can clean up the Conduit logs, so I can add a setup issue for deprecated method calls, so I can remove deprecated methods, so I can get rid of `DifferentialRevisionListData`, so I can make Differntial policy-aware.
Adds modern infrastructure and UI to all of the Conduit interfaces (except only partially for the logs, that will be the next diff).
Test Plan:
{F48201}
{F48202}
{F48203}
{F48204}
{F48206}
This will get further updates in the next diff:
{F48205}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6331
Summary:
Depends on D6329. This fixes `http://www.example.com/D123`, which currently gets the "D123" rendered, after addition of the Asana rule. It also removes a hack for object refernces.
Basically, the "hyperlink" rule needs to happen after rules which specialize hyperlinks (Youtube, Asana) but before rules which apply to general text (like the Differential and Maniphest rules). Allow these rules to specify that they have higher or lower priority.
Test Plan: Asana rules, Differential rules and Diffusion rules now all markup correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6330
Summary: See discussion in IRC. Not 100% sure what's going on here because of email ghost theives, but conceivably a commit with no changes will end up with `null` changesets instead of `array()` changesets, which throws. Such diffs are certianly possible (`git commit --allow-empty`) even if they aren't the issue in this specific case. See T3416. Initialize changesets to `array()` to avoid throwing.
Test Plan:
Viewed some commits?
iiam
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6339
Summary:
Addes a button group to filter tasks by agents, non-agents or all.
Fixes T3394
Test Plan: View task list, filter by agents, filter by non agents. Make sure the correct tasks display.
Reviewers: epriestley, dctrwatson
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3394
Differential Revision: https://secure.phabricator.com/D6328
Summary: Fixes T3400. Users are crafty. Attempt to outwit them.
Test Plan: Added all kinds of nonsense to my PATH to hit all the errors. Verified sensible-looking error messages which I couldn't figure out any way to misread or outwit.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3400
Differential Revision: https://secure.phabricator.com/D6318
Summary: I have simply copied the existing css into the oblivious skin. I don't know if this is the right approach (code duplication), but considering this skin should be isolated (and will potentially differ) I think this makes sense.
Test Plan: Use a code block on a phame post.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Differential Revision: https://secure.phabricator.com/D6313
Summary:
Fixes T3436. Currently, when installs have configuration options which we don't know about, we raise a fairly confusing/ambiguous message about the options being unknown. Instead:
- Keep a list of previously valid (but now deleted) config, with explanatory reasons for what happened to it. Present this information, along with altenate wording ("Obsolete Config" instead of "Unknown Config") where applicable.
- Show a list of all the places the config is defined.
- Provide an active link to delete it from the web UI.
- Provide a command to delete it from the CLI.
- Allow `bin/config delete` to delete configuration options which no longer have a definition.
Test Plan:
- Set an auth key in database, local and file config.
- Walked through the setup issue, cleaning it up.
- Set an invalid key and made sure I still got a reasonable error (this now has better cleanup instructions).
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3436
Differential Revision: https://secure.phabricator.com/D6317
Summary: Currently you can't refresh the live blog or a blog post after clicking "View Live" due to POST action. I have removed the setRenderAsForm call on the "View Live" actions. I am unsure if this has any unintended consequences but I have tested and not found any.
Test Plan: Click the "View Live" action within a blog post or blog, and observe that the request occurs via GET not POST
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6314
Summary: Ref T2852. Setting followers (like CCs) is a separate API call, but we don't need to do anything complicated.
Test Plan: Synchronized revisions and verified the parent task got followers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6308
Summary:
Ref T2852.
Before trying related users, try using the feed story's actor. This is the most correct voice to act in.
Test Plan: Ran `feed/republish`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6305
Summary:
Ref T2852.
The parent task is open unless the revision is in the states "closed" or "abandoned". If it's in "needs review", it remains open. This last bit is slightly unlike Differential, but consistent with the Google Doc and generally seems like a better fit. There's no way to put the task in a "Waiting on Others" state in Asana like we can in Differential.
The subtasks are closed unless the revision is in the state "needs review". This is generally consistent with Differential.
Test Plan:
Made a series of changes to a revision and synchronized it repeatedly:
- requested changes
- commandeered
- requested review
- abandoned
Verified task and subtasks synchronized states correctly in Asana.
{F47554}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6304
Summary:
Ref T2852. Depends on D6302. This now creates, destroys, and synchronizes subtasks.
- After finishing the parent task stuff, we pull a list of all known subtasks.
- We load all those subtasks.
- If we fail to load any, we delete their objects and edges on the Phabricator side.
- Of the remaining subtasks, we find subtasks for users who aren't related to the object any more and delete them in Asana and locally (for example, if alincoln is removed as a reviewer, we delete his subtask).
- For all the related users, we either synchronize their existing task or create a new one for them.
- Then we write edges for any new tasks we added.
This doesn't handle a few weird edge cases in any specific way:
- If a subtask is moved under a different parent, we ignore it.
- If a new subtask is created that we don't know about, we ignore it.
- If a subtask we know about is deleted, we just respawn it. This is consistent with "DON'T EDIT THESE". You can force sync to stop by deleting the parent.
Addititionally:
- Make the "don't edit" warning more compelling and visceral.
Test Plan:
- Kind of ran it a bit.
- There are like 3,000 edge cases here so this is hard to test exhaustively.
- Forced a few of the edge cases to happen.
- Nothing seems immediately broken in an obvious way?
{F47551}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6303
Summary:
Ref T2852. This is highly incomplete but seems structurally sound. Some additional context is available in the Google doc.
- Add a workspace ID configuration. Without it, nothing else activates.
- Add a worker which reacts to feed stories.
- Feed stories about things which aren't Differential objects are ignored.
- We load the revision, or fail permanently if we can't.
- We get all the related user PHIDs (author, reviewers, CCs).
- We check if any of them have linked Asana accounts, or fail permanently if they don't.
- We check for an "ASANATASK" edge from the revision.
- If we do not find one, we create a new task.
- If we do find one, we load the task.
- If we succeed, we check the chronological key of the most recent synchronized feed story ("cursor").
- If this story is the same or newer, we update the task to synchronize it to the current state of the revision.
- If we fail to load the task, we fail permanently ("asana task has been deleted").
- We then publish the actual story text to the task.
Not in yet:
- Updating followers requires separate API calls which we don't do yet.
- No subtasks yet.
- No sync of open/closed state.
Test Plan: {F47546}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6302
Summary: Currently if disqus is used and a phame post is loaded over ssl, the disqus api is not loaded over https. This fixes that by honouring the protocol being used by the html document.
Test Plan: Open a phame post over https
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6311
Summary: Ref T2852. Reduce the number of magical strings in use, and prepare the Asana bridge for eventual workspace/project support (a little bit).
Test Plan: Verified enriched links still work properly.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6301
Summary:
Ref T2852. Add a `log()` method to `PhabricatorWorker` to make debugging easier.
I renamed the similar Drydock-specific method.
Test Plan:
Used logging in a future revision:
...
<<< [36] <http> 211,704 us
Updating main task.
>>> [37] <http> https://app.asana.com/api/1.0/tasks/6153776820388
...
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6296
Summary:
Ref T1536. Ref T2852. Currently, after refreshing the token we don't actually return it. This means that code relying on token refresh fails once per hour (for Asana) in a sort of subtle way. Derp.
Update `bin/auth refresh` to make this failure more clear.
Test Plan: Set `force refresh` flag and verified a return value.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536, T2852
Differential Revision: https://secure.phabricator.com/D6295
Summary:
Ref T2852.
- Broadly, we support "I have a Ref, I need a PHID" well but not "I have a PHID, I need a Ref".
- Add DoorkeeperExternalObjectQuery, and use it to query ExternalObjects.
- Allow external objects to be imported by their internal PHIDs. Basically, if we have an edge pointing at an ExternalObject, we can say "load all the data about this" from just the PHID and have it hit all the same code.
- Allow construction of Refs from ExternalObjects. This makes the "I have a PHID, I need a Ref" easier.
Test Plan:
- Verified Asana links still enrich properly at display time.
- Used in future revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6294
